Skip to content

QL language reference: variables must be lowerId #8930

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 1 commit into from
May 5, 2022

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Apr 28, 2022

To prepare for a future QL language change where variable names must start with a lower-case letter, this commit updates the QL language reference (including the language specification) to change the variable name grammar from simpleId to lowerId.

To find the locations I should change, I grepped for variable and simpleId. I also looked manually through the reference section on variables.

To prepare for a future QL language change where variable names must
start with a lower-case letter, this commit updates the QL language
reference (including the language specification) to change the variable
name grammar from `simpleId` to `lowerId`.
@jbj jbj requested review from alexet and jf205 April 28, 2022 07:18
@jf205
Copy link
Contributor

jf205 commented Apr 28, 2022

Thanks @jbj. I guess in theory there could be example CodeQL snippets in the documentation that use upper case variable names, but maybe they're not so easy to find. Let's assume there are very few and do ad hoc fixes if we spot any (or they are reported by users). I'll leave the proper review to @alexet.

I can't remember when the variable name change actually comes into effect. Be aware that the docs team updates codeql.github.com/docs for every CLI release, so we shouldn't merge this until we are in the release cycle where this change lands.

cc @felicitymay

@jbj
Copy link
Contributor Author

jbj commented Apr 29, 2022

I guess in theory there could be example CodeQL snippets in the documentation that use upper case variable names, but maybe they're not so easy to find. Let's assume there are very few and do ad hoc fixes if we spot any (or they are reported by users).

I commented yesterday on the internal issue about how I've looked for upper-case variables in our documentation. In summary, I think I've checked 80-90% of them.

I can't remember when the variable name change actually comes into effect. Be aware that the docs team updates codeql.github.com/docs for every CLI release, so we shouldn't merge this until we are in the release cycle where this change lands.

If we stick with the timeline in this issue, we can start introducing warnings for upper-case variables on May 15. That means we should introduce those warnings in main within a week or so, and then they'll be in the next CLI release. Given that we're so close, I suggest merging this documentation change right away. It means that the compiler will be less strict than the spec for a few weeks, but I don't see a problem with that.

@felicitymay
Copy link
Contributor

Thanks for the mention @jf205. We're in the process of publishing the docs for CodeQL CLI 2.9.1, so merging into main is likely to result in the changes being published for CodeQL CLI 2.9.2.

@jbj jbj merged commit d747c6e into github:main May 5, 2022
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.

4 participants