SONARPY-2189 Represent Django views in the updated project level symbol table #2121
Conversation
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2189
branch
7 times, most recently
from
16137ac to
436dc9a
Compare
guillaume-dequenne-sonarsource
force-pushed
the
MMF-3945
branch
2 times, most recently
from
fafc00f to
1c837cc
Compare
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2189
branch
2 times, most recently
from
a43d63c to
31dc566
Compare
guillaume-dequenne-sonarsource
changed the title
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2189
branch
4 times, most recently
from
d905db9 to
d017d01
Compare
guillaume-dequenne-sonarsource
requested a review
from maksim-grebeniuk-sonarsource
guillaume-dequenne-sonarsource
force-pushed
the
MMF-3945
branch
from
759e9d2 to
a5e22df
Compare
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2189
branch
3 times, most recently
from
952f2e5 to
849a3dd
Compare
| .filter(symbol -> symbol.fullyQualifiedName() != null) | ||
| .ifPresent(symbol -> djangoViewsFQN.add(symbol.fullyQualifiedName())); | ||
| PythonType pythonType = viewArgument.expression().typeV2(); | ||
| if (pythonType instanceof UnknownType.UnresolvedImportType unresolvedImportType) { |
There was a problem hiding this comment.
Since we will need to add FQN for other types (at least for ClassType) I suggest to introduce an interface like HasFullyQualifiedName, and implement it for the UnresolvedImportType and for the FunctionType. Then this if-else will be simplified
There was a problem hiding this comment.
As discussed, UnresolvedImportType does not have a fully qualified name, because we said fully qualified name is strictly the name where a type is defined, and not just a re-exported name. Because the type is unknown, it has an importPath instead, which I think is good to keep as a clearly distinct concept from fullyQualifiedName.
In this case, I think what is misleading is probably the fact that I kept djangoViewsFqns when it probably should be djangoViewsImportPaths (fullyQualifiedName being seen here as a more precise import path).
Wdyt?
If we agree on that, I can rename the variable for djangoViewsFqns. As for the HasFullyQualifiedName interface, I agree it will make sense when introduced for ClassType, though. I'd probably introduce it at the same time as ClassType however.
There was a problem hiding this comment.
As discussed, UnresolvedImportType does not have a fully qualified name, because we said fully qualified name is strictly the name where a type is defined, and not just a re-exported name. Because the type is unknown, it has an importPath instead, which I think is good to keep as a clearly distinct concept from fullyQualifiedName.
In this case, I think what is misleading is probably the fact that I kept djangoViewsFqns when it probably should be djangoViewsImportPaths (fullyQualifiedName being seen here as a more precise import path).
Wdyt?
If we agree on that, I can rename the variable for djangoViewsFqns. As for the HasFullyQualifiedName interface, I agree it will make sense when introduced for ClassType, though. I'd probably introduce it at the same time as ClassType however.
| def ambiguous(): ... | ||
| else: | ||
| def ambiguous(): ... | ||
| urlpatterns.append(path('bar', ambiguous, name='bar')) |
There was a problem hiding this comment.
Minor: here is definition of the urlpatterns is missed which could confuse reading this test case
| String content = """ | ||
| from django.urls import conf | ||
| import views | ||
| urlpatterns = [conf.path('foo', views.foo, name='foo'), conf.path('baz')] |
There was a problem hiding this comment.
Same here
There was a problem hiding this comment.
Here urlpatterns is defined though? Are you referring to something else?
There was a problem hiding this comment.
Ooops, I misread it, yes, here is ok
| @@ -135,4 +138,8 @@ public PythonType owner() { | |||
| public TypeOrigin typeOrigin() { | |||
| return typeOrigin; | |||
| } | |||
|
|
|||
| public String fullyQualifiedName() { | |||
There was a problem hiding this comment.
Since now we have let's say two types having fqn (the UnresolvedImportType and the FunctionType and in nearest future we may want to add it to more types - I think it makes sense to introduce an interface like:
public interface HasFullyQualifiedName {
String fullyQualifiedName();
}There was a problem hiding this comment.
As mentioned in the previous comment, UnresolvedImportType does not have a fqn.
We could have another interface that returns importPath (which would be the FQN for FunctionType and ClassType). I'm not yet sure about that though, as I'm afraid it could be misused.
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2189
branch
2 times, most recently
from
e9da24a to
5690f4e
Compare
guillaume-dequenne-sonarsource
requested a review
from maksim-grebeniuk-sonarsource
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2189
branch
from
5690f4e to
d7986c7
Compare
|
SONARPY-2189