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
Refactor the code printing explanation for unification errors #1496
Conversation
The changes look ok. However, in term of refactoring, I would rather go one step further: define a concrete type for unification explanations, and split |
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.
See minor Changes comment, but otherwise the patch is fine and I approve of it.
Florian's proposal is reasonable but it also sounds like a lot more work (because the author of the patch would have to understand each explanation and painfully track its data dependencies). I will follow what @Armael thinks is best here, but I would be of the opinion that we could merge this now, and consider this major rewrite as a separate step.
Changes
Outdated
@@ -38,6 +38,9 @@ Working version | |||
(Arthur Charguéraud and Armaël Guéneau, with help from Gabriel Scherer and | |||
Frédéric Bour) | |||
|
|||
- GPR#1493 : Refactor the code printing explanation for unification type errors, | |||
in order to avoid duplicating pattern matches (Armaël Guéneau) |
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 credits should be on their own line.
18b1baf
to
802406e
Compare
Indeed, @Octachron proposition means the explanation would be computed earlier (when checking if there is one), and one would have to check that this does not interferes with code that happens between the current |
You may have done something unexpected while rebasing your branch to fix the Change entry because the current patchset shown by Github is not at all what I would expect. |
802406e
to
436eb13
Compare
Oops, sorry. Should be fixed now. |
Changes
Outdated
@@ -38,6 +38,10 @@ Working version | |||
(Arthur Charguéraud and Armaël Guéneau, with help from Gabriel Scherer and | |||
Frédéric Bour) | |||
|
|||
- GPR#1493 : Refactor the code printing explanation for unification type errors, | |||
in order to avoid duplicating pattern matches | |||
(Armaël Guéneau) |
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.
", review by Florian Angeletti and Gabriel Scherer"?
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.
indeed, sorry. fixed.
ef67aae
to
eb6982e
Compare
eb6982e
to
cede2c3
Compare
This patches refactor
explanation
/has_explanation
to avoid duplicating the pattern matches, the first time to check if there is an explanation, and the second time to actually get it. Instead,explanation
now returns a(Format.formatter -> unit) option
; in theSome
case one can call the closure to compute and print the explanation.