10000 bpo-39031: Include elif keyword when producing lineno/col-offset info for if_stmt by lysnikolaou · Pull Request #17582 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-39031: Include elif keyword when producing lineno/col-offset info for if_stmt #17582

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
Merged
merged 4 commits into from
Dec 12, 2019
Merged

bpo-39031: Include elif keyword when producing lineno/col-offset info for if_stmt #17582

merged 4 commits into from
Dec 12, 2019

Conversation

lysnikolaou
Copy link
Member
@lysnikolaou lysnikolaou commented Dec 12, 2019 •
edited by miss-islington
Loading

When parsing an "elif" node, lineno and col_offset of the node now point to the "elif" keyword and not to its condition, making it consistent with the "if" node.

https://bugs.python.org/issue39031

Automerge-Triggered-By: @pablogsal

@lysnikolaou lysnikolaou mentioned this pull request Dec 12, 2019
Closed
@pablogsal
Copy link
Member

Hi there 😉 Can you add a NEWS entry?

@pablogsal pablogsal self-requested a review December 12, 2019 20:59
@pablogsal pablogsal self-assigned this Dec 12, 2019
pablogsal
pablogsal requested changes Dec 12, 2019
View reviewed changes
Copy link
Member
@pablogsal pablogsal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you also add a elif case to the exec_test list and regenerate the test data (check the end of the file for instructions)?

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

lysnikolaou and others added 2 commits December 12, 2019 22:12
@lysnikolaou @pablogsal
Update News file based on feedback
1adf59d 
Co-Authored-By: Pablo Galindo <Pablogsal@gmail.com>
@lysnikolaou
Include an elif case in exec_test
4fc6d4f
@lysnikolaou
Copy link
Member Author

Thanks a lot for the review @pablogsal! I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.

pablogsal
pablogsal approved these changes Dec 12, 2019
View reviewed changes
Copy link
Member
@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @lysnikolaou! I will backport this to 3.8 and 3.7 once the CI finish.

@miss-islington
Copy link
Contributor

@lysnikolaou: Status check is done, and it's a failure ❌ .

@miss-islington
Copy link
Contributor

@lysnikolaou: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 025a602 into python:master Dec 12, 2019
@miss-islington
Copy link
Contributor

Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

@miss-islington
Copy link
Contributor
miss-islington comme 8000 nted Dec 13, 2019

Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @lysnikolaou, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 025a602af7ee284d8db6955c26016f3f27d35536 3.8

@miss-islington
Copy link
Contributor

Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

GH-17589 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 13, 2019
@lysnikolaou @miss-islington
bpo-39031: Include elif keyword when producing lineno/col-offset info…
e09db81 
… for if_stmt (pythonGH-17582)

When parsing an "elif" node, lineno and col_offset of the node now point to the "elif" keyword and not to its condition, making it consistent with the "if" node.

https://bugs.python.org/issue39031

Automerge-Triggered-By: @pablogsal
(cherry picked from commit 025a602)

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
pablogsal added a commit that referenced this pull request Dec 13, 2019
@pablogsal @lysnikolaou
[3.7] bpo-39031: Include elif keyword when producing lineno/col-offse…
0ed45d0 
…t info for if_stmt (GH-17582) (#17584)

When parsing an "elif" node, lineno and col_offset of the node now point to the "elif" keyword and not to its condition, making it consistent with the "if" node.

https://bugs.python.org/issue39031

Automerge-Triggered-By: @pablogsal.
(cherry picked from commit 025a602)

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
pablogsal pushed a commit that referenced this pull request Dec 13, 2019
@miss-islington @lysnikolaou @pablogsal
bpo-39031: Include elif keyword when producing lineno/col-offset info…
3b18b17 
… for if_stmt (GH-17582) (GH-17589)

When parsing an "elif" node, lineno and col_offset of the node now point to the "elif" keyword and not to its condition, making it consistent with the "if" node.

https://bugs.python.org/issue39031

Automerge-Triggered-By: @pablogsal
(cherry picked from commit 025a602)

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
@lysnikolaou
Copy link
Member Author

@pablogsal I just found out that I kind of messed this up and one more change is needed. What is the right course of action here? Open a new PR for the additional change?

@pablogsal
Copy link
Member

Open a new PR for the additional change?

Yes, also explain in the issue what is missing

This was referenced Dec 14, 2019
Merged
Merged
@asottile
Copy link
Contributor
asottile commented Jan 8, 2020 •
edited
Loading

@pablogsal In the past these usually don't get backported to already-released versions as they break static analysis tools (I just hit an error due to this patch when upgrading from 3.8.0 to 3.8.1+!)

@pablogsal
Copy link
Member

@pablogsal In the past these usually don't get backported to already-released versions as they break static analysis tools (I just hit an error due to this patch when upgrading from 3.8.0 to 3.8.1+!)

@asottile Apologies for the inconvenience :S. In this case I backported it because I considered this a bugfix but I can see how this can be inconvenient for static analysers.

Although static analysers would break anyway with a new major release I will make sure to take this into account for further similar situations so we don't cause breakage for minor releases.

@asottile
Copy link
Contributor
asottile commented Jan 8, 2020

@pablogsal In the past these usually don't get backported to already-released versions as they break static analysis tools (I just hit an error due to this patch when upgrading from 3.8.0 to 3.8.1+!)

@asottile Apologies for the inconvenience :S. In this case I backported it because I considered this a bugfix but I can see how this can be inconvenient for static analysers.

Although static analysers would break anyway with a new major release I will make sure to take this into account for further similar situations so we don't cause breakage for minor releases.

yep makes sense, in major versions breakage is ~kinda expected -- but having conditions for < (3, 8, 1) is pretty unfortunate

oh actually it appears this isn't released in 3.8.1 so it would be part of 3.8.2? should we revert instead?

@pablogsal
Copy link
Member
pablogsal commented Jan 8, 2020 •
edited
Loading

oh actually it appears this isn't released in 3.8.1 so it would be part of 3.8.2? should we revert instead?

Unless I am mistaken, the fixes for issue39031 are in 3.8.1 and the latest 3.7.x because we are relying on that for developing pegen.

@asottile
Copy link
Contributor
asottile commented Jan 8, 2020

oh actually it appears this isn't released in 3.8.1 so it would be part of 3.8.2? should we revert instead?

Unless I am mistaken, the fixes for issue39031 are in 3.8.1 because we are relying on that for developing pegen.

ah yep they are -- false alarm. I'm bad at reading git log 😆

@pablogsal
Copy link
Member

Notice that this fixes are also on the latest 3.7.x :(

@asottile
Copy link
Contributor
asottile commented Jan 8, 2020

ugh that's even messier :(

@pablogsal
Copy link
Member

@asottile But notice this is not unique of this issue, for example https://bugs.python.org/issue38535 also changes ast colum offsets and line numbers and was backported (for example, is in 3.7.6).

@asottile
Copy link
Contributor
asottile commented Jan 8, 2020

bpo-16806 was not for instance -- and there was a change to the tokenizer in a patch version which pretty badly broke pyflakes + pycodestyle but I'm having a difficult time finding it (it involved ENDMARKER and files not ending in newlines)

ideally we'd document this somewhere and be more consistent 🤷‍♂️

@lysnikolaou lysnikolaou mentioned this pull request Jan 9, 2020
Merged
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@lysnikolaou @shihai1991
bpo-39031: Include elif keyword when producing lineno/col-offset info…
eb3e050 
… for if_stmt (pythonGH-17582)

When parsing an "elif" node, lineno and col_offset of the node now point to the "elif" keyword and not to its condition, making it consistent with the "if" node.


https://bugs.python.org/issue39031



Automerge-Triggered-By: @pablogsal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal pablogsal approved these changes

Assignees

@miss-islington miss-islington

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lysnikolaou @pablogsal @bedevere-bot @miss-islington @asottile @the-knights-who-say-ni
0