dune icon indicating copy to clipboard operation
dune copied to clipboard

Add dune command to get the contents of a "corrected" file

Open TyOverby opened this issue 4 years ago • 13 comments

Desired Behavior

Instead of looking at the diff output for expect-tests in the terminal, I'd like to run my own diff program to see the change in the files before promoting. In order to do this, I'd need a dune command to fetch the full contents of the "corrected" file.

Example

$ dune get-corrected ./src/foo.ml

<corrected file output here>

TyOverby avatar Oct 21 '20 14:10 TyOverby

You can specify a custom diff program with dune --diff-command=CMD. Have you tried this?

nojb avatar Oct 21 '20 14:10 nojb

what commands accept the --diff-command flag?

TyOverby avatar Oct 22 '20 15:10 TyOverby

All of them, I think, but in particular dune build, dune runtest, etc.

nojb avatar Oct 22 '20 15:10 nojb

Have you been able to try this out? Can we close the issue?

nojb avatar Oct 29 '20 07:10 nojb

Ah sorry, I haven't been able to test it out yet. I'm concerned about what would happen if dune is run using "watch" mode though. My goal is to build good tooling around expect-test diff visualization in vim, and I'd hate for every keystroke to trigger an invocation of vimdiff

TyOverby avatar Oct 29 '20 15:10 TyOverby

OK, I understand what you are asking better now. What kind of expect tests are you using specifically?

nojb avatar Oct 29 '20 20:10 nojb

What kind of expect tests are you using specifically?

I'm not sure entirely what you mean by this; I use https://github.com/janestreet/ppx_expect and some other not-publicly-available tooling that also plops down ".corrected" files next to files that need to have their contents adjusted.

TyOverby avatar Oct 30 '20 00:10 TyOverby

I'm not sure entirely what you mean by this; I use https://github.com/janestreet/ppx_expect ...

dune has several different features that can be called "expect tests". Using ppx_expect is just one of them.

One (albeit not very satisfactory) way to get hold of the .corrected file when using ppx_expect inline tests (after it has been built by dune) is to go fish for it inside the _build/default directory, in the same place where the original .ml file is found.

Having a dedicated dune get-corrected command for files that can be promoted is an interesting suggestion, but it needs to be evaluated in terms of the general semantics of dune, its usability, etc.

@jeremiedimino @rgrinberg what do you think?

nojb avatar Oct 30 '20 05:10 nojb

(I should precise that I was the one who suggested to Ty to open this ticket, this came up during a discussion we had about the Jane Street vim plugin and the transition to Dune).

Yes, such a command seems good to me. If we had the rpc alongside the command line interface ready, I'd say that unless this is something we expect users to call manually from the terminal we should add this feature to the RPC only. In this case, it seems reasonable to expect that users would call this command in the terminal.

FTR, the various features that produce correction files all rely on the diff or diff? actions. So all corrections are recorded through the same system.

ghost avatar Nov 02 '20 10:11 ghost

Having a dedicated dune get-corrected command for files that can be promoted is an interesting suggestion, but it needs to be evaluated in terms of the general semantics of dune, its usability, etc.

What about making it a subcommand of describe? dune describe corrected sounds a little better to me.

rgrinberg avatar Nov 03 '20 20:11 rgrinberg

dune describe returns a structured output, while here we want the plain file dumped on stdout.

ghost avatar Nov 04 '20 11:11 ghost

Should dune get-corrected trigger a build before showing the corrected files? Or should it behave like dune promote which only acts on the results of the last build? The latter makes more sense to me.

nojb avatar Nov 10 '20 09:11 nojb

Me too. When we have the RPC, perhaps this command should connect to the running Dune if it detects one to get the most up-to-date result. But for now doing the same as dune promote seems good.

ghost avatar Nov 10 '20 12:11 ghost

@emillon do you think this is worth adding to dune promotion?

rgrinberg avatar Dec 21 '22 20:12 rgrinberg

Is this still relevant?

nojb avatar Mar 11 '23 11:03 nojb

I think so. It would be simple enough to add and quite useful

rgrinberg avatar Mar 11 '23 16:03 rgrinberg

I could give this a try but I'd need a bit of guidance to start.

  • Has anyone already started to work on this? If so then I'll find sometihng else to do.
  • Do I understand that the idea is to add something along the lines of dune promotion show <file-path>? Instead of showing the current state of the file (as cat would) or the diff (as dune promotion diff would) it shows the state as it would be after applying the promotion (as dune promotion apply; cat <file> would but without the side effect)?
  • Where in the source do I start?

raphael-proust avatar Sep 30 '23 13:09 raphael-proust

@raphael-proust

  • Do I understand that the idea is to add something along the lines of dune promotion show <file-path>?

I think so.

Instead of showing the current state of the file (as cat would) or the diff (as dune promotion diff would) it shows the state as it would be after applying the promotion (as dune promotion apply; cat <file> would but without the side effect)?

Yes, this should be simple enough as promotions already get registered someplace. In the _build/default directory you would find the updated file. The promotion mechanism then just copies it to the source tree.

  • Where in the source do I start?

The promotion subcommand is in bin/promotion.ml. There you can see it uses src/dune_engine/diff_promotion.ml which should provide everything you need to a new subcommand. The mlis contain useful documentation.

Alizter avatar Sep 30 '23 15:09 Alizter