-
Notifications
You must be signed in to change notification settings - Fork 189
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
[ScopeProvider] Expose more granular Assignments and Accesses for dotted imports #284
Conversation
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
=======================================
Coverage 93.96% 93.96%
=======================================
Files 219 219
Lines 21245 21288 +43
=======================================
+ Hits 19962 20004 +42
- Misses 1283 1284 +1
Continue to review full report at Codecov.
|
This is a pretty good enhancement. Import |
""" | ||
|
||
after = """ | ||
import a.b, a.b.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a.b.c
is not removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand a.b.c may have side effect and thus is not safe to remove. My question is how do we not remove it?
The ScopeVisitor looks like will use name a.b
to store access to assignment a.b
.
Is it because import a.b.c
creates three assignment lookup entries (a, a.b, a.b.c), a.b
access is recorded to the second one so, the import is not removed?
In the case of x.y.z
, we only associate access x.y.z
to import x.y.z
instead of import x.y
(which are x and x.y).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because a.b.c
is looked at in isolation (without considering the existence of the import for a.b
).
So essentially because of the same reason this is not fixed:
import a
import a
a
This is a limitation of the RemoveUnusedImportsVisitor
, it only removes unused imports, not duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great.
One last thing, can you add a test case to cover this line?
https://codecov.io/gh/Instagram/LibCST/pull/284/diff?src=pr&el=tree#D2-642...643
Add a from module import *
in a test case should make it covered.
Summary
This PR adds extra
Assignment
s created by theScopeProvider
for dotted imports likeimport a.b.c
. For such an import, there are now three assignments, keyed bya
,a.b
, anda.b.c
. For accesses, only the most specific access is recorded and attached to the appropriate assignment.Test Plan
Added unit tests.
Fixes #281.