SONARPY-2316 Resolve field members of a ClassType #2163
Conversation
maksim-grebeniuk-sonarsource
force-pushed
the
SONARPY-2316
branch
4 times, most recently
from
c91153d
to
18232f2
Compare
maksim-grebeniuk-sonarsource
requested a review
from guillaume-dequenne-sonarsource
| @@ -70,6 +70,13 @@ public SymbolImpl(String name, @Nullable String fullyQualifiedName) { | |||
| this.kind = Kind.OTHER; | |||
| } | |||
|
|
|||
| public SymbolImpl(String name, @Nullable String fullyQualifiedName, @Nullable String annotatedTypeName) { | |||
There was a problem hiding this comment.
minor you can slightly modify the constructor above to call the new one you ve created.
public SymbolImpl(String name, @Nullable String fullyQualifiedName) {
this(name, fullyQualifiedName, null);
}| @@ -70,6 +70,13 @@ public SymbolImpl(String name, @Nullable String fullyQualifiedName) { | |||
| this.kind = Kind.OTHER; | |||
| } | |||
|
|
|||
| public SymbolImpl(String name, @Nullable String fullyQualifiedName, @Nullable String annotatedTypeName) { | |||
There was a problem hiding this comment.
question It seems there are only a few unit tests for this class. May be you can add a unit test in SymbolImplTest to test the constructor you have created?
| scan(assignmentStatement.assignedValue()); | ||
| scan(assignmentStatement.annotation()); | ||
| scan(assignmentStatement.variable()); |
There was a problem hiding this comment.
any reason you don't call super? Tests in TypeInferenceV2Test are still green if I use the call to super
| scan(assignmentStatement.assignedValue()); | |
| scan(assignmentStatement.annotation()); | |
| scan(assignmentStatement.variable()); | |
| super.visitAnnotatedAssignment(assignmentStatement); |
| .filter(NameImpl.class::isInstance) | ||
| .map(NameImpl.class::cast) | ||
| .ifPresent(lhsName -> { | ||
| if (currentType() instanceof ClassType) { |
There was a problem hiding this comment.
question this line is to check that you are in a class definition right?
There was a problem hiding this comment.
discussed
| class A: | ||
| test = "hi" | ||
| test = True |
There was a problem hiding this comment.
I suggest that you update the test with type hint to ensure that the type of test Symbol is bool in the end.
| class A: | |
| test = "hi" | |
| test = True | |
| class A: | |
| test : str = "hi" | |
| test : bool = True |
There was a problem hiding this comment.
test added
| @@ -562,10 +631,13 @@ class B: | |||
| test = "hi" | |||
| class C(A, B): | |||
| pass | |||
| C.test | |||
There was a problem hiding this comment.
why do you have to remove the access to the field test everywhere?
There was a problem hiding this comment.
discussed
maksim-grebeniuk-sonarsource
force-pushed
the
SONARPY-2316
branch
from
5d5a23c
to
aa163b8
Compare
|
SONARPY-2316