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 is_annotation for types used in classdef base and assign value #406

Merged
merged 4 commits into from
Oct 28, 2020

Conversation

luciawlli
Copy link
Contributor

Summary

fix is_annotation for types used in classdef base and assign value
discussion

Test Plan

tox -e py37
tox -e autofix

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 21, 2020
@luciawlli luciawlli requested a review from zsol October 21, 2020 05:33
libcst/metadata/scope_provider.py Outdated Show resolved Hide resolved
libcst/metadata/scope_provider.py Show resolved Hide resolved
@@ -647,6 +647,7 @@ def __init__(self, provider: "ScopeProvider") -> None:
Union[cst.Call, cst.Annotation, cst.Subscript]
] = set()
self.__in_ignored_subscript: Set[cst.Subscript] = set()
self.__ignore_annotation: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be a stack or a counter instead of a simple bool to keep track of nesting. Let's add a nested test case while we're at it.

libcst/metadata/scope_provider.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

The logic in visit_Call also needs to be updated.
The Aug of TypeVar and NewType are not actually Annotation.

Comment on lines 725 to 730
def visit_Assign_value(self, node: cst.Assign) -> None:
self.__ignore_annotation += 1

def leave_Assign_value(self, node: cst.Assign) -> None:
self.__ignore_annotation -= 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Handle Assign differently doesn't seem right.
We should only consider the is_annotation for access in Annotation node (__in_annotation already takes case that).

@codecov-io
Copy link

Codecov Report

Merging #406 into master will decrease coverage by 0.00%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
- Coverage   94.11%   94.11%   -0.01%     
==========================================
  Files         231      231              
  Lines       22427    22449      +22     
==========================================
+ Hits        21108    21128      +20     
- Misses       1319     1321       +2     
Impacted Files Coverage Δ
libcst/metadata/scope_provider.py 96.69% <86.66%> (-0.35%) ⬇️
libcst/metadata/tests/test_scope_provider.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01c8098...849eb86. Read the comment docs.

@zsol zsol merged commit a1b1ae4 into master Oct 28, 2020
@lpetre lpetre deleted the is_annotation branch November 19, 2021 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants