Skip to content

bpo-42864: Improve error messages regarding unclosed parentheses#24161

Merged
pablogsal merged 5 commits into
python:masterfrom
pablogsal:parens
Jan 19, 2021
Merged

bpo-42864: Improve error messages regarding unclosed parentheses#24161
pablogsal merged 5 commits into
python:masterfrom
pablogsal:parens

Conversation

@pablogsal

@pablogsal pablogsal commented Jan 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

https://bugs.python.org/issue42864

With this PR:

❯ cat lel.py
( 1+2
❯ ./python lel.py
  File "/home/pablogsal/github/python/master/lel.py", line 1
    ( 1+2
    ^
SyntaxError: '(' was never closed

~/github/python/master parens*
❯ cat ../a.py
x = {
~/github/python/master parens*
❯ ./python ../a.py
  File "/home/pablogsal/github/python/master/../a.py", line 1
    x = {
        ^
SyntaxError: '{' was never closed

@pablogsal
pablogsal requested a review from lysnikolaou January 8, 2021 03:04
@pablogsal

pablogsal commented Jan 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I opened this as a draft to evaluate the idea. This probably needs #24140 to be fixed first for the tests to pass

@tirkarthi

tirkarthi commented Jan 8, 2021

Copy link
Copy Markdown
Member

cc: @asottile

@pablogsal
pablogsal force-pushed the parens branch 5 times, most recently from 823ba3e to 83572fc Compare January 14, 2021 22:47
@pablogsal

pablogsal commented Jan 14, 2021

Copy link
Copy Markdown
Member Author

@lysnikolaou Do you mind taking a look at this when you have some time? I hope we can find some way to make this work 🤞

pablogsal
pablogsal commented Jan 14, 2021
View reviewed changes
Comment thread Lib/test/test_codeop.py Outdated

@pablogsal pablogsal Jan 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The EOF was reached in a different line before but now it raises the same error as the case without newlines (because the unopen parentheses are at the same place). I think we can allow that to happen.

@pablogsal
pablogsal force-pushed the parens branch 2 times, most recently from 2a7d535 to b5bfa50 Compare January 14, 2021 23:04
@pablogsal
pablogsal marked this pull request as ready for review January 14, 2021 23:15
@pablogsal

pablogsal commented Jan 18, 2021

Copy link
Copy Markdown
Member Author

I would like this PR to be exposed as soon as possible to new users testing 3.10 so we can find as many problems with this as possible (if there are any), given that this actively exercises the code added in #24140. If nobody can review this by the end of the week I would like to go ahead and land it and we can tweak it later unless someone has concerns with this.

@lysnikolaou

lysnikolaou commented Jan 18, 2021

Copy link
Copy Markdown
Member

I'm so sorry for not replying sooner. I got distracted by other stuff and forgot about this. I'll find some time to review it by Wednesday evening. I hope that's okay and sorry again.

@pablogsal

pablogsal commented Jan 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I hope that's okay and sorry again.

Absolutely! Don't worry, I totally understand and there is no problem. Also, don't feel pressured to review this if you don't have time. I wrote the message in case that nobody has time to review this in the short term because I wanted some early exposure for this :)

Also, tell me if you need more time to go through this and I can hold it for more time

lysnikolaou
lysnikolaou approved these changes Jan 19, 2021
View reviewed changes

@lysnikolaou lysnikolaou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! 🚀 Great work!

Do the Windows warnings bother us or is it okay to merge with those?

@pablogsal

pablogsal commented Jan 19, 2021

Copy link
Copy Markdown
Member Author

Do the Windows warnings bother us or is it okay to merge with those?

I would merge and then investigate the proper way to fix it. It probably requires a cast or changing the type of the parent stack but maybe it requires changes somewhere else due to type inconsistency across these fields and I prefer to investigate separately if that's ok with you :)

@lysnikolaou

lysnikolaou commented Jan 19, 2021

Copy link
Copy Markdown
Member

Do the Windows warnings bother us or is it okay to merge with those?

I would merge and then investigate the proper way to fix it. It probably requires a cast or changing the type of the parent stack but maybe it requires changes somewhere else due to type inconsistency across these fields and I prefer to investigate separately if that's ok with you :)

Yup. Go!

@pablogsal
pablogsal merged commit d6d6371 into python:master Jan 19, 2021
@pablogsal

pablogsal commented Jan 20, 2021

Copy link
Copy Markdown
Member Author

Yup. Go!

Let's see how many weird edge cases we found after this 😆

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@pablogsal @adorilson
bpo-42864: Improve error messages regarding unclosed parentheses (pyt…
f775cbf 
…honGH-24161)
@pablogsal
pablogsal deleted the parens branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lysnikolaou lysnikolaou lysnikolaou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pablogsal @tirkarthi @lysnikolaou @the-knights-who-say-ni @bedevere-bot