Skip to content

Port remaining Desugar and Applications errors to the new scheme #8263

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

Merged
merged 4 commits into from
Feb 18, 2020

Conversation

ausmarton
Copy link
Contributor

As described in #1589
Tried porting a couple more of the Errors from Desugar. There are a couple more still remaining, but they seemed to take a bit longer to reproduce the errors for a minimal test case.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@ausmarton ausmarton force-pushed the ticket/1589 branch 4 times, most recently from ba5f039 to 75224a5 Compare February 10, 2020 14:36
@anatoliykmetyuk anatoliykmetyuk self-assigned this Feb 10, 2020
@anatoliykmetyuk anatoliykmetyuk self-requested a review February 10, 2020 16:14
@ausmarton ausmarton force-pushed the ticket/1589 branch 3 times, most recently from ac00dbb to e1edca9 Compare February 10, 2020 22:10
@ausmarton ausmarton changed the title Port more Desugar errors to the new scheme Port more Desugar and Applications errors to the new scheme Feb 10, 2020
@ausmarton ausmarton force-pushed the ticket/1589 branch 5 times, most recently from 5df66aa to bbe845d Compare February 14, 2020 23:32
@ausmarton
Copy link
Contributor Author

ausmarton commented Feb 16, 2020

Tried porting couple more error messages from Desugar, and came across a redundant check for extension methods being done in Parsers. I've removed the check from Parsers now as suggested by @smarter here: gitter.im/lampepfl/dotty?at=5e485ace0c50da598c1236e9
It seems like that has some implications on #6900

@ausmarton ausmarton requested a review from smarter February 16, 2020 03:25
@ausmarton ausmarton force-pushed the ticket/1589 branch 4 times, most recently from fb56784 to 24a8ba1 Compare February 17, 2020 01:32
@ausmarton ausmarton changed the title Port more Desugar and Applications errors to the new scheme Port remaining Desugar and Applications errors to the new scheme Feb 17, 2020
@ausmarton ausmarton force-pushed the ticket/1589 branch 3 times, most recently from 0893738 to 7a3d106 Compare February 17, 2020 11:09
@ausmarton
Copy link
Contributor Author

All Desugar errors ported to the new scheme now.

I've noticed that for some reason, although the checks are successful, this page seems to show that the "continuous-integration/drone/pr" check is still not reported. In fact, the latest build here: https://dotty-ci.epfl.ch/lampepfl/dotty/4482 is successful.

@ausmarton ausmarton force-pushed the ticket/1589 branch 2 times, most recently from ebe8850 to 697f6e7 Compare February 17, 2020 18:05
Copy link
Contributor

@anatoliykmetyuk anatoliykmetyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @ausmarton!

@anatoliykmetyuk anatoliykmetyuk merged commit 76caf35 into scala:master Feb 18, 2020
@ausmarton ausmarton deleted the ticket/1589 branch February 18, 2020 21:05
@odersky
Copy link
Contributor

odersky commented Mar 2, 2020

I had to back out most of this. It conflicted in many ways with the preceding PR #8318.

@anatoliykmetyuk In the future, let's not merge error message commits in areas that are still in flux, in particular if they have pending PRs.

Also, let's forget about error messages tests for these kinds of things. I see no value for these tests, and they cause a huge amount of friction for everyone who has to change things in the future.

@ausmarton If you want to try to port to error messages based on the new PR, please go ahead. Please keep in mind to keep things short & simple. I think the advice in #1589 is overblown - following it to the letter slows down our work rather than helping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants