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.

Sign up for GitHub

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

Jump to bottom

Add location to type matching errors #270

Merged
merged 8 commits into from
Nov 4, 2021
Merged

Add location to type matching errors #270

merged 8 commits into from
Nov 4, 2021

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Oct 29, 2021 •
edited
Loading

@xsebek
Copy link
Member Author

xsebek commented Oct 29, 2021

There might be other escape routes or a better way to annotate the error, but his seems to help. 🤷‍♂️

@byorgey
Copy link
Member

byorgey commented Oct 29, 2021

This looks OK, but is there any reason not to add a single call to addLocToTypeError at the very top of inferModule? As it is, it will only catch any errors that happen specifically on the two lines where you added it.

@xsebek
Copy link
Member Author

xsebek commented Oct 29, 2021

@byorgey nope, I was just afraid you would beat me to it, so I pushed the first thing that worked 😄

@byorgey
Copy link
Member

byorgey commented Oct 29, 2021

nope, I was just afraid you would beat me to it

lol, I was actually working on it when you pushed this, so well done 😆

@restyled-io restyled-io bot mentioned this pull request Oct 29, 2021
Closed
@restyled-io restyled-io bot mentioned this pull request Oct 29, 2021
Closed
@xsebek
Copy link
Member Author

xsebek commented Oct 29, 2021

The error scope for type mismatch seems to be reasonable now. 🥳
But I better add some unit tests now that it is no longer a two line change 😅

@xsebek xsebek changed the title Add location to def escaping type error Add location to type mathing errors Oct 29, 2021
@xsebek xsebek changed the title Add location to type mathing errors Add location to type matching errors Oct 30, 2021
@xsebek xsebek mentioned this pull request Nov 4, 2021
Merged
@restyled-io restyled-io bot mentioned this pull request Nov 4, 2021
Closed
@byorgey
Copy link
Member

byorgey commented Nov 4, 2021

LGTM!

@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Nov 4, 2021
@mergify mergify bot merged commit fb79910 into main Nov 4, 2021
@mergify mergify bot deleted the catch-mismatch branch November 4, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Report error in def body only
2 participants
@xsebek @byorgey