Skip to content

bpo-43950: support some multi-line expressions for PEP 657 #27339

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 2 commits into from
Jul 25, 2021

Conversation

isidentical
Copy link
Member

@isidentical isidentical commented Jul 24, 2021

This is basically something that I noticed up while fixing test runs for another issue. It is really common to have multiline calls, and when they fail the display is kind of weird since we omit the annotations. E.g;

 $ ./python t.py
Traceback (most recent call last):
  File "/home/isidentical/cpython/cpython/t.py", line 11, in <module>
    frame_1()
    ^^^^^^^^^
  File "/home/isidentical/cpython/cpython/t.py", line 5, in frame_1
    frame_2(              
  File "/home/isidentical/cpython/cpython/t.py", line 2, in frame_2
    return a / 0 / b / c
           ~~^~~
ZeroDivisionError: division by zero

This patch basically adds support for annotating the rest of the line, if the instruction covers multiple lines (start_line != end_line).

https://bugs.python.org/issue43950

Automerge-Triggered-By: GH:isidentical

@isidentical isidentical changed the title bpo-43950: support multi-line expressions if the intruction covers th… bpo-43950: support some multi-line expressions for PEP 657 Jul 24, 2021
@isidentical isidentical force-pushed the bpo-43950-multiline branch from ff6022e to 8d8136f Compare July 24, 2021 18:52
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.

I recall they we had this version implemented but we did abandoned it because it looked weird in done scenarios. I am missing anything or is the fact that we are stopping in whitespace enough to account for the problems with the old approach?

@isidentical
Copy link
Member Author

The old approach was highlighting the whole line if the end_offset is greater than 256 (-1) but where the instruction was only covering a single line. This is about when an expression covers multiple lines (e.g the call above). Since we now the start point, and we know for a fact that the instruction continue to next lines we can just highlight the line to the end without any false positives.

@pablogsal
Copy link
Member

Can you add some extra tests with some specific cases like function calls spawning multiple lines as well as binops spawning multiple lines (for example)? I will like to cover this with more cases if possible,

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.

LGTM modulo my comment regarding more tests

@miss-islington
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

5 participants