-
Notifications
You must be signed in to change notification settings - Fork 373
Rewrite the conflict explanation extraction mechanism #4349
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
Conversation
Really amazing! I tried it quickly locally and I have a few comments. Before:
After:
I feel like the second bullet point could be removed for an even shorter conflict message. However I suppose this might be needed for much difficult constraints. I'm also not sure this is a good idea to tell users about
or something like that? I'm not sure what would be the best sentence here but I'm sure we can find something. I also think this could be said only once as supposed to twice. |
Related issue: #4171 |
[ERROR] Package conflict! | ||
* No agreement on the version of core: | ||
- core != 112.17.00 | ||
- core != 112.17.00 |
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.
what does it mean? :-)
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.
The message is off, but there is actually little we can easily do about it :/ ; my opinion is that in this case the error is clear anyway so it's still OK (but yeah, we should at least filter out duplicate lines).
As for the explanation why, this is a different instance of #2229: we are working on a package graph where we can't really distinguish between conjunction and disjunction between edges. In this case, we have core<112.17.00 & core>112.17.00
, which actually resolves to false
, but without knowing about the &
we wrongly reduce it as an |
, which reduces to core != 112.17.00
.
I'll check if I can find a way to detect such cases, and skip the "formula reduction" when they happen.
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.
ha yes I was talking about the duplication -- but you are right, it's a bit hard to suggest something more actionnable to the user here.
I totally agree... but I couldn't find a generic way to determine that the first message is strictly better and supersedes the second... I already have quite a few heuristics to remove (lots of) spurious or secondary messages, but here the reason why the first is better is not totally clear, especially without resorting to independent knowledge of the package graph. We have a similar case here, where actually the "missing dependency" is extra information too. In your case, there is another message issue though: a
Indeed, it might help if you're stuck but may not be the best advice in general. I at least removed the duplication, but am open to removing or changing the message as well (with your suggestion, unless something better comes up ?) Thanks! |
Finally! This needs more testing, so please everyone gather your
extreme conflict messages and send them to me; but this is very
promising.