Skip to content

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

Merged
merged 11 commits into from
Mar 15, 2022

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Mar 11, 2022

TODO: Wait for #8323.

Adds a QL-for-QL queries that puts every name definitions in two categories:

  • Things that should start with an upper-case letter.
  • Things that should start with a lower-case letter.

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

@erik-krogh erik-krogh marked this pull request as ready for review March 14, 2022 11:25
@erik-krogh erik-krogh requested review from a team as code owners March 14, 2022 11:25
Copy link
Contributor

@jbj jbj left a 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.

atorralba
atorralba previously approved these changes Mar 14, 2022
Copy link
Contributor

@atorralba atorralba left a 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]>
Copy link
Contributor

@jbj jbj left a 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,.

Copy link
Member

@RasmusWL RasmusWL left a 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 {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

JS 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants