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

bpo-43797: Improve syntax error for invalid comparisons #25317

Merged
merged 4 commits into from
Apr 12, 2021
Merged

bpo-43797: Improve syntax error for invalid comparisons #25317

merged 4 commits into from
Apr 12, 2021

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 9, 2021 •
edited by bedevere-bot
Loading

@pablogsal pablogsal requested a review from lysnikolaou as a code owner April 9, 2021 19:13
@pablogsal pablogsal force-pushed the bpo-43797 branch 2 times, most recently from a36abe8 to 8d7971c Compare April 9, 2021 19:17
@pablogsal pablogsal requested a review from gvanrossum April 9, 2021 19:19
@pablogsal pablogsal force-pushed the bpo-43797 branch 6 times, most recently from 527808d to e23664c Compare April 9, 2021 21:01
gvanrossum
gvanrossum reviewed Apr 10, 2021
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

This inspired me to point out a whole lotta nits with the phrasing of the errors. But basically I think this is an improvement, so if you don't feel like fixing all those, that's fine with me. (Maybe check with Serhiy again.)

Lib/test/test_generators.py Outdated
@@ -2013,7 +2013,7 @@ def printsolution(self, x):
>>> def f(): (yield bar) = y
Traceback (most recent call last):
...
SyntaxError: cannot assign to yield expression
SyntaxError: cannot assign to yield expression. Maybe you meant '==' instead of '='?
Copy link
Member

Choose a reason for hiding this comment

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

Here and elsewhere it feels jarring that the initial sentence ("cannot ...") doesn't start with a capital letter, but is followed by a period and another sentence that does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I can update the error message with a capital letter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, on second thought, the capital letter technically is the "S" in SyntaxError no?

Lib/test/test_genexps.py
@@ -103,7 +103,7 @@
>>> dict(a = i for i in range(10))
Traceback (most recent call last):
...
SyntaxError: invalid syntax
SyntaxError: invalid syntax. Maybe you meant '==' or ':=' instead of '='?
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the suggestion here is very useful. More likely some other misunderstanding or typo happened.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this particular example yes, but imagine something like all(x = y for x in elements for y in other_elements). As it makes more sense depending on the actual function, is going to be hard to contextualize.

pablogsal and others added 2 commits April 10, 2021 21:43
@pablogsal @gvanrossum
Update Lib/test/test_fstring.py
80a727c 
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
@pablogsal
Apply review comments
c952132
gvanrossum
gvanrossum approved these changes Apr 10, 2021
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! I agree on your pushback.

@pablogsal pablogsal merged commit b86ed8e into python:master Apr 12, 2021
@bedevere-bot
Copy link

@pablogsal: Please replace # with GH- in the commit message next time. Thanks!

@pablogsal pablogsal deleted the bpo-43797 branch April 12, 2021 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pablogsal @bedevere-bot @gvanrossum @the-knights-who-say-ni