0957b8b to
9f8316c
Compare
0284c2a to
33f03d9
Compare
There was a problem hiding this comment.
I suspect this was incorrect before (being adjusted by -1 due to the multiline string).
For instance, here's the ast dump of (using astpretty)
x = 1 + 1$ python ~/workspace/astpretty/astpretty.py t.py
Module(
body=[
Assign(
lineno=1,
col_offset=0,
targets=[Name(lineno=1, col_offset=0, id='x', ctx=Store())],
value=BinOp(
lineno=1,
col_offset=4,
left=Num(lineno=1, col_offset=4, n=1),
op=Add(),
right=Num(lineno=1, col_offset=8, n=1),
),
),
],
)
You'll notice that the BinOp and its left (Num) have the same col_offset
There was a problem hiding this comment.
Just a question: after fixing this, when a < line_start possible?
There was a problem hiding this comment.
I would think it's not possible now, though I don't know of all the cases for every other token. I think tqs are the only multiline token (since iirc escaped newlines are not a token)
There was a problem hiding this comment.
I don't understand this function well.
int lines = LINENO(parent) - 1;
int cols = parent->n_col_offset;
These value are get from parent, before this loop:
/* Find the full fstring to fix location information in `n`. */
while (parent && parent->n_type != STRING)
parent = parent->n_child;
Is this OK?
There was a problem hiding this comment.
yeah I think so, what's happening here (and the variable names aren't super great):
From reading #1800 (where this function was introduced) it was discussed briefly but I didn't pick up the rationale for that loop there. When I removed / adjusted it during testing it broke all of the f-string column / offset tests so I think it's necessary.
|
(I've rebased to regen importlib to resolve the conflict) |
|
(I've rebased to regen importlib to resolve the conflict) |
|
Diff and tests looks good to me. @ericvsmith would you review this? You seems reviewed #1800. |
|
I'll take a look, but it might take me a while to get to it. Unfortunately I have a project that's due at the end of the year that's taking up all of my time. So while I'll try, you might not want to wait for me. I'm working on a fix to bpo-34364 that will completely back out the changes implemented in #1800. I'm not sure if that will interact with this change, though. |
|
(I've rebased to regen importlib to resolve the conflict) Commands for me for next time since I expect this to not be the last: git fetch --all
git checkout [bpo-16806](https://bugs.python.org/issue16806)
git reset --hard asottile/[bpo-16806](https://bugs.python.org/issue16806)
git pull --rebase origin master
git checkout origin/master -- Python/importlib* # when rebase fails
make regen-importlib
git add Python
git rebase --continue
git push asottile HEAD -f |
|
urgh, the bot edited my comment and broke my code block :( |
|
(I've rebased to regen importlib to resolve the conflict) Please take a look, this is tedious -- I'd like to be done with this branch! |
|
(I've rebased to regen importlib to resolve the conflict) Please take a look, this is tedious -- I'd like to be done with this branch! |
|
happy new year! friendly ping :) |
|
changes to the ast generally shouldn't be backported because they break linters / code formatters which make assumptions about how minor versions of python work in this particular case, this was discussed on bpo if you want more information |
|
@asottile Sorry, should've read that better! Thanks! |
I revived the patch listed in the linked bpo issue.
https://bugs.python.org/issue16806