-
Notifications
You must be signed in to change notification settings - Fork 351
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
Fix some of the unhelpful conflict messages by merging formulas that include each other #5210
Conversation
# Return code 50 # | ||
### opam install --show ppx_expect.v0.14.0 | ||
[ERROR] No switch is currently set. Please use 'opam switch' to set or install a switch | ||
# Return code 50 # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for developers: this test was included in #4504 for the same purpose but has never tested anything
…include each other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
not available because the package is pinned to version 4.12.0 | ||
* Missing dependency: | ||
- expect >= 0.0.6 -> batteries -> ocaml < 4.12.0 | ||
- ocaml < 4.12.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these conflict message tests, it can be helpful to know what is the "intended output". I'm not sure if it is always simple to know in advance.
Here if we know that it is ocaml < 4.12.0
the conflict that we want to highlight (i suppose that from the pin above), a comment may help to know that the fix is good (and not a simplification that outputs the wrong message).
This case is simple thought to determine thought.
@@ -1024,6 +1022,7 @@ let extract_explanations packages cudfnv2opam reasons : explanation list = | |||
not (List.exists (function `Conflict (_,_,has_invariant) -> has_invariant | _ -> false) explanations) | |||
in | |||
let msg = `Conflict (msg1, msg2, msg3) in | |||
(* TODO: remove this List.mem *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(* TODO: remove this List.mem *) | |
(* TODO: remove this List.mem, cf [explanations] below *) |
The review highlighted a small issue i would like to fix and understand first so I'm closing this one in favor of #6106 |
I spent today trying to fix one of the issue mentionned in the first comment of #4373 reproduced (somewhat) by:
which gives:
for some reason this is a bit different than the comment mentionned in #4373 but I suppose this is caused by the the multiple pins --current. In any case this sort of issue happens all the time on macOS/arm64 as shown by #4373 (comment)
The goal of the change is to merge similar
Missing
cases. I know @AltGr mentioned that the code already filters something but I couldn’t find that part of the code and figured it would be fine to duplicate this, even if just for debugging purpose.With this change
opam install expect.0.0.6
shows the expected: