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.

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

Fix some aststrip import bugs #4994

Merged
merged 2 commits into from May 4, 2018

Conversation

Projects
None yet
2 participants
@msullivan
Copy link
Collaborator

msullivan commented May 2, 2018

Strip assignments in ImportAll nodes and clear names even
if there are assignments.

Diff looks bigger than it really is because some conditional code was made unconditional. Suggest ignoring whitespace changes when looking at it, which github just added a handy checkbox under "diff settings" to do!

Fix some aststrip import bugs
Strip `assignments` in ImportAll nodes and clear `names` even
if there are assignments.

@msullivan msullivan requested review from JukkaL and ilevkivskyi May 2, 2018

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Here are some comments:

@@ -1588,6 +1588,39 @@ x = ''
==
==

[case testImportStarSomethingMoved1]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 3, 2018

Collaborator

1 at the end of test case name implies you either forgot to add second tests or forgot to remove this 1.

This comment has been minimized.

@msullivan

msullivan May 3, 2018

Author Collaborator

It was the latter, though I do also kind of like always having numbers on tests, because it means you don't need to rename the original one to add a second.

@@ -233,6 +228,11 @@ def visit_import_all(self, node: ImportAll) -> None:
del self.names[name]
node.imported_names = []

def visit_for_stmt(self, node: ForStmt) -> None:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 3, 2018

Collaborator

It looks like we don't have visit_with_stmt while it also might need to be stripped (at least for consistency).

This comment has been minimized.

@msullivan

msullivan May 3, 2018

Author Collaborator

Hm probably target_type needs to get cleared. I'll look at it for a different PR

@@ -1588,6 +1588,39 @@ x = ''
==
==

[case testImportStarSomethingMoved1]
[file p.py]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 3, 2018

Collaborator

You never import p so I am not sure anything is actually checked in this test.

This comment has been minimized.

@msullivan

msullivan May 3, 2018

Author Collaborator

testfinegrained picks up all the files involved rather than following imports. I'll add an import though to make it more clear, since most tests still do that.

node.relative,
name)
if sym:
self.names[imported_name] = sym

def visit_import(self, node: Import) -> None:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 3, 2018

Collaborator

I see logically three changes, but only two tests added. Maybe there is missing a test with a simple import? (i.e. the one visited by this method)

This comment has been minimized.

@msullivan

msullivan May 3, 2018

Author Collaborator

The change was for consistency, but it turns out that Import can't even have assignments. Changing it to an assert instead.

# If the node is unreachable, don't reset entries: they point to something else!
if node.is_unreachable: return
if self.names:
# Reset entries in the symbol table. This is necessary since

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 3, 2018

Collaborator

Maybe explain in comment why we need to always do this independently of assignments?

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

OK, looks good now!

@msullivan msullivan merged commit 6ab826d into master May 4, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the import_conflict branch May 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.