Accept correction files produced by ppx_driver #415
Conversation
| Build.progn | ||
| [ build | ||
| ; Build.return | ||
| (A.promote_if |
There was a problem hiding this comment.
Is it really worth introducing a module alias just for this one use?
There was a problem hiding this comment.
It's just that super_context defines an Action module as well, so we get a name clash
| | Some _ | None -> false | ||
|
|
||
| let promote_correction ~uses_ppx_driver fn build = | ||
| if not uses_ppx_driver then |
There was a problem hiding this comment.
Small nit: I find else clauses in if (not ...) confusing and prefer if not x then foo else bar to turn to if x then bar else foo.
There was a problem hiding this comment.
Personally, I find if/match expressions easier to read when the simple choice is first. That's why I wrote it this way
src/action.ml
Outdated
| @@ -744,7 +745,8 @@ let rec exec t ~ectx ~dir ~env_extra ~stdout_to ~stderr_to = | |||
| match mode with | |||
| | Always -> files | |||
| | If_corrected_file_exists -> | |||
| List.filter files ~f:(fun file -> Path.exists file.Promote.src) | |||
| List.filter files ~f:(fun { Promote. src; dst } -> | |||
| Path.exists src && Path.exists dst) | |||
There was a problem hiding this comment.
Why do we need to make sure that the destination exists now?
There was a problem hiding this comment.
Basically we must not promote the file if it's generated. But maybe we should do that check in super_context.ml instead
There was a problem hiding this comment.
Thinking about this more, doing nothing when the source file doesn't exist seems wrong. For instance if the user generates a file containing some [@@deriving_inline ...] attribute, then we at least want to check that they are correct.
I guess promote-if could have the following behavior: print the diff but only promote if the source file exist as well.
|
I did the change for promote-if. I also adapted the code so that |
|
actually that's not necessary, the worse that can happen is that ppx_driver will try to run |
ghost
force-pushed
the
ppx-driver-corrected
branch
from
4ccfae3 to
ae13072
Compare
|
We can merge this now then, I submitted a new minor release of ppx_driver: ocaml/opam-repository#11236 |
This PR makes
[@@deriving_inline ...]works with jbuilder. It requires: janestreet-deprecated/ppx_driver#17