-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: Multiple django improvements #5139
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
Conversation
Which we currently don't handle :( Also added a bit more explanatory comments
Before it the class would contain _all_ functions xD
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 like nice improvements (good catch that djangoRouteHandlerFunctionTracker
still tracks all functions).
@@ -2078,7 +2099,7 @@ private module Django { | |||
DjangoRouteHandler() { | |||
exists(DjangoRouteSetup route | route.getViewArg() = djangoRouteHandlerFunctionTracker(this)) | |||
or | |||
any(DjangoViewClassDef vc).getARequestHandler() = this | |||
any(DjangoViewClass vc).getARequestHandler() = this |
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.
Should this (and below) use PossibleDjangoViewClass
?
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.
Nope. PossibleDjangoViewClass
might be a bit too strongly worded. Maybe it should have been called ProbablyNotDjangoViewClass
😆
} | ||
|
||
/** A class that might be a django View class. */ | ||
class PossibleDjangoViewClass extends Class { |
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.
Is the name meant to make me think of the Range
pattern?
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.
nope.
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.
Thanks for the explanations. I am still not quite sure of the role of/need for PossibleDjangoViewClass
. It seems like a not-meant-to-use-further extension point.
Apart from this, I am happy to merge this.
The role of
Since we no longer want to restrict the classes considered for tracking
The sole purpose of Hopefully this makes things more clear. Otherwise let's talk on zoom. If you do get an aha moment, would probably be cool with a docs suggestion so it's more clear for the next person 🤷 |
After discussion with @yoff
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.
Thanks, LGTM
This PR covers 3 improvements to our django modeling, so we now handle:
.as_view()
in routing setup, but without known view super-typeself.request
,self.args
, andself.kwargs
on view classI can be persuaded to split this into 3 PRs, I just found it easy to do them together. Will strongly recommend going through changes commit-by-commit