Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a diff command asking for files whitout rule for their generation #2396

Closed
clecat opened this issue Jul 10, 2019 · 6 comments
Closed

Add a diff command asking for files whitout rule for their generation #2396

clecat opened this issue Jul 10, 2019 · 6 comments

Comments

@clecat
Copy link

clecat commented Jul 10, 2019

Hello, I'm currently working on mdx, one of the part of the project generates .correctedfiles for further promotion.
However, I have encountered a problem in the tests:
When mdx does not generate any .corrected file in the case where the test would want it to do so, dune runtest does not raise any errors, and the problem is just ignored:

Considering a test running mdx which is supposed to generate a .corrected file:

(alias
 (name   runtest)
 (deps   (:x file.md) (:y file.md.expected)
             (package mdx))
 (action (progn
           (run ocaml-mdx test %{x})
           (diff? %{y} %{x}.corrected))))

If ocaml-mdx doesnt generate the correction, diff? will ignore it, the problem will not be raised and I cant use diff as I dont have any rules for the generation of the corrected file.
I could solve it by creating a rule executing mdx and having the corrected file in it's targets and then use diff instead of diff?, which is I suppose the current adviced approach.

But as diff? was specifically done for this kind of tests, it feels kinda weird not to use it, also, my problem can lead to bugs, as other people on other project could not be aware of this dilemma and use diff? anyway.

This is why I was thinking that adding a third diff command could be a nice addition: An hybrid diff which does not ask the rules for the generation of the files (like diff?) but which throws if the files does not exist (like diff).

What do you think about it ? (I might have been a bit unclear, but I would be glad to discuss about it)

@ghost
Copy link

ghost commented Jul 10, 2019

diff? was introduced for actions that produce a target that is used in the compilation pipeline and might optionally generate a correction, such as ppx rewriters. Adding a third variant seems to me like it would complicate things. As you mentioned, systematically generating the correction and using diff is the right way to go.

Maybe the documentation could be improved though. A PR clarifying all this would definitely be welcome.

@clecat
Copy link
Author

clecat commented Jul 10, 2019

I see, thanks for the answer, I'll continue with forcing the generation

@ghost
Copy link

ghost commented Jul 10, 2019

Alright, I'm closing this PR then. Feel free to re-open it if you find another issue with this method.

@samoht
Copy link
Member

samoht commented Jul 10, 2019

@diml maybe something simpler would be to change the semantics of diff? to only require the first argument to be optionally present ; the second one will be mandatory. I guess it might break existing usage of diff? but the semantics would be much closer to what is needed for the workflow you are describing.

@ghost
Copy link

ghost commented Jul 10, 2019

That would work yes. However, it should be the second argument that is optional given that the corrected file comes second. We can enable that new behaviour when using the dune 2.x language

@samoht
Copy link
Member

samoht commented Jul 10, 2019

sounds good!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants