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

Always generate .corrected files when used in dune #168

Closed
NathanReb opened this issue Sep 11, 2019 · 9 comments
Closed

Always generate .corrected files when used in dune #168

NathanReb opened this issue Sep 11, 2019 · 9 comments

Comments

@NathanReb
Copy link
Contributor

While toying around with mdx.1.4.0 I stumbled upon the bug fixed by #136. This bug is now fixed but one thing I noticed is that it manifested in a really weird and disturbing way if you used ocaml-mdx rule to generate your mdx test rules.

Because the generated rule uses (diff? %{x} %{x}.corrected) actions, it will silently pass if no .corrected file has been generated.

I'm wondering if it wouldn't be more robust to have ocaml-mdx rule to produce a rule that generates all the .corrected no matter what, using the --force-output option of ocaml-mdx test and to produce strict diff rules thus preventing any such silent failures.

Let me know what you think!

@samoht
Copy link
Collaborator

samoht commented Sep 11, 2019

See ocaml/dune#2482

@NathanReb
Copy link
Contributor Author

I'm not sure it's related as the issue here was that nothing happened because the second argument of diff? was missing (ie wasn't generated by MDX). I might be misunderstanding this but I don't think the new diff? will change that.

@NathanReb
Copy link
Contributor Author

In other words diff? behaved as it was supposed to but that made it hard to even know there was a bug in the first place.

@samoht
Copy link
Collaborator

samoht commented Sep 12, 2019

Isn't this a similar issue that @clecat had in ocaml/dune#2396 ?

But if the real issue is that mdx doesn't generate a .corrected file when it should have, it will be better to just fix that issue instead of trying to work-around possible bugs.

@NathanReb
Copy link
Contributor Author

As I said, the bug is already fixed. I'm suggesting generating a more explicit rule that would be less error prone and would allow us to catch those bugs earlier if they arise.

@clecat
Copy link
Contributor

clecat commented Sep 12, 2019

Indeed this is the same issue I had several weeks ago.
I'm not sure ocaml-mdx rule should forcefully generate those outputs as it could be bothersome for the users (having promotions triggered even when we have no real diffs)
But I would be really glad if we could have diff commands more appropriate for that kind of check (my original request was rejected, but maybe with a little bit more of argumentation, we could make it done)

@NathanReb
Copy link
Contributor Author

Note that if the file are identical dune won't promote anything, that's how promotion through the diff action works, whether the .corrected is present or not doesn't change anything.

@NathanReb
Copy link
Contributor Author

My point being, always generating the .corrected would be transparent for users.

@NathanReb
Copy link
Contributor Author

I'm closing this as:

  1. it is not entirely true, Jenga still relies on this behaviour (i.e. running the diff ourself in mdx)
  2. we now have a dune stanza which forces that behaviour by passing --force-output anyway

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

3 participants