-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rename all upper-case variables, and all lower-case modules #8403
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
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.
LGTM except some small comments about the QL query. We'll also need review from the JavaScript team and the Python team. If there's any resistance to renaming those modules, I'm happy to defer that part to a later PR and just focus on the variables for now.
javascript/ql/lib/change-notes/2022-02-07-deprecated-modules.md
Outdated
Show resolved
Hide resolved
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.
Java LGTM 👍
Co-authored-by: Nick Rolfe <[email protected]>
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.
LGTM, but we'll also need approval from @github/codeql-javascript and @github/codeql-python,.
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.
Looks ok for Python.
A bit unfortunate that there was not consistency between the renaming, and we got DjangoImpl
, InvokeModule
, and DjangoMod
, but as I said in inline comment, that's sort of a nitpick, and it's for code that we want to rewrite anyway, so meh 🤷
@@ -20,14 +20,14 @@ private module Invoke { | |||
API::Node invoke() { result = API::moduleImport("invoke") } | |||
|
|||
/** Provides models for the `invoke` module. */ | |||
module invoke { | |||
module InvokeModule { |
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.
NIT: Would have been nice with consistency between what it was called for django (DjangoImpl
) and here. But 🤷 since this is something we want to rewrite anyway.
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'll change it if I get a change request from JS, otherwise I'll just keep it.
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.
JS 👍🏻
TODO: Wait for #8323.Adds a QL-for-QL queries that puts every name definitions in two categories:
Every violation of the above is fixed as part of this PR, luckily almost no code violated the above.
A few modules in the public API started with a lower-case letter, I've left a deprecated alias for those.
/cc @jbj