-
Notifications
You must be signed in to change notification settings - Fork 44
Add context to some error messages #2017
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 context to some error messages #2017
Conversation
| :: HasCallStack | ||
| => Ord variable | ||
| => InternalVariable variable | ||
| => [(SomeVariable variable, TermLike variable)] |
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.
I see Ana left a "TODO" comment on line 431 about a posible refactoring. I could take care of it. What do you think @ana-pantilie @ttuegel ?
(I'm not able to comment on that line)
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.
We already have unsafeWrapFromAssignments, and it looks like the major difference between these two is that unsafeWrap doesn't enforce the variable renaming invariant. I believe the question here is if we want to be able to create a Substitution like this, even for testing (where unsafeWrap is most widely used). @ttuegel what do you think? Is there any reason to keep unsafeWrap the way it is?
Reviewer checklist
stack test --coveragestack haddock