Enforcing consistent casing of acronyms #8323

erik-krogh · 2022-03-03T18:25:59Z

Acronyms should be PascalCase/camelCase.

I made a patch for converting the casing of acronyms to PascalCase.
At first I thought it would just be a fun day-of-learning exercise that shouldn't be merged.
However, naming conflicts started showing up (see list below), and now I think this PR should be merged to avoid e.g. having both a XmlComment and XMLComment class in the same scope.

Conflicts detected from this:

There are a few places where only the casing on names were different, and those started conflicting with this patch (because they got imported into the same scope).
They are:

JS: Two (effectively identical) implementations of isDomRootType(..). (one, two). I simply deleted one of them.
Java: Two identical implementations of UrlOpenStreamMethod. (one, two). One of those were on a .ql file, so that one just got deleted.
C#: Two XmlComment classes that did very different things. (one, two). One of them could be renamed to XmlCommentLine, which I think is a more fitting name.

The code in this PR:

This PR first introduces a QL-for-QL query for detecting use of upper-case acronyms in names.
Then some of those got automatically patched (see backref)
- a deprecated alias for the old name were added.
- QLDoc that mentions one of the renaming got updated.
- Only some acronyms got pached, the backref contains a list of the acronyms that were patched.
Lastly the code got fixed to make all the tests pass again (including fixing the conflicts).

Leftovers

There are some leftovers that I haven't renamed yet (classes in XML.qll / YAML.qll).
Those are both used in internal tests, and thus require a little more care when merging, so I'm skipping that for now.

I also didn't rename any files.

tausbn · 2022-03-03T21:50:24Z

ql/ql/src/codeql_ql/style/AcronymsShouldBeCamelCaseQuery.qll

+  name.regexpMatch(".*[A-Z]{4,}.+") and
+  not node.hasAnnotation("deprecated") and
+  // allowed upper-case acronyms.
+  not name.regexpMatch(".*(PEP|AES|DES|EOF).*")


We should probably also ignore the T that prefixes some newtype branches. For instance, TQLDoc is perfectly valid according to the style guide.

igfoo · 2022-03-04T13:04:44Z

cpp/ql/lib/semmle/code/cpp/PrintAST.qll


 /**
 * The query can extend this class to control which functions are printed.
 */
-class PrintASTConfiguration extends TPrintASTConfiguration {
+class PrintAstConfiguration extends TPrintASTConfiguration {


Shouldn't TPrintASTConfiguration also be changed?

Yes, I didn't flag the newtype name itself in the QL-for-QL query, I only flagged the branches.
Fixed with 32b1d98

geoffw0 · 2022-03-04T17:42:04Z

Just checking: no public interfaces are affected? Hence, no change note required?

erik-krogh · 2022-03-04T19:08:18Z

Just checking: no public interfaces are affected? Hence, no change note required?

Ehhh. Lots of public interfaces are affected, but the old name is a deprecated alias for the new name.
So everything should still work.
I'm not sure if that requires a change-note.

geoffw0 · 2022-03-07T10:32:15Z

If people are going to start seeing deprecated warnings they weren't seeing before, this definitely requires a change note.

hvitved

C# 👍

hvitved · 2022-03-09T08:44:43Z

csharp/ql/lib/semmle/code/asp/WebConfig.qll

@@ -7,67 +7,88 @@ import csharp
 /**
 * A `Web.config` file.
 */
-class WebConfigXML extends XMLFile {
-  WebConfigXML() { this.getName().matches("%Web.config") }
+class WebConfigXml extends XMLFile {


Why is XMLFile not also changed?

Ah, I guess that is because of what you mentioned in the Leftovers section.

Ah, I guess that is because of what you mentioned in the Leftovers section.

Exactly. I'll probably look at that in a followup.

erik-krogh · 2022-03-11T10:19:44Z

I did a rebase on main to fix merge-conflicts.
The automated patch is reapplied in patch upper-case acronyms to be PascalCase.
Everything else should be the same.

nickrolfe

Some minor Ruby nitpicks, but otherwise LGTM.

ruby/ql/lib/change-notes/2022-02-07-deprecated-acronyms.md

ruby/ql/lib/codeql/ruby/frameworks/Files.qll

Co-authored-by: Nick Rolfe <[email protected]>

aschackmull · 2022-03-31T10:54:50Z

I noticed some mistakes here where Java class names occurring in qldoc was accidentally renamed. Fix: #8625

github-actions bot added QL-for-QL C# C++ Java JS Python Ruby labels Mar 3, 2022

erik-krogh force-pushed the acronyms branch from 98b3e9c to c2c9780 Compare March 3, 2022 19:50

erik-krogh added the no-change-note-required This PR does not need a change note label Mar 3, 2022

erik-krogh force-pushed the acronyms branch from 1863b3d to b43184d Compare March 3, 2022 21:23

tausbn reviewed Mar 3, 2022

View reviewed changes

erik-krogh force-pushed the acronyms branch 3 times, most recently from acd31f1 to a37291e Compare March 4, 2022 08:42

erik-krogh mentioned this pull request Mar 4, 2022

JS: Refactor the XSS / Client-side-url queries #8304

Merged

erik-krogh force-pushed the acronyms branch from a37291e to 9a3870f Compare March 4, 2022 10:03

erik-krogh marked this pull request as ready for review March 4, 2022 12:48

erik-krogh requested a review from a team as a code owner March 4, 2022 12:48

erik-krogh requested a review from a team March 4, 2022 12:48

erik-krogh requested review from a team as code owners March 4, 2022 12:48

erik-krogh changed the title ~~QL: add query detecting upper-case acronyms~~ Enforcing consistent casing of acronyms Mar 4, 2022

igfoo reviewed Mar 4, 2022

View reviewed changes

erik-krogh dismissed annarailton’s stale review via 656de2b March 7, 2022 12:46

hvitved reviewed Mar 9, 2022

View reviewed changes

erik-krogh added 12 commits March 11, 2022 11:05

QL: add query detecting upper-case acronyms

e3a1579

patch upper-case acronyms to be PascalCase

69353bb

C#: rename XmlComment to XmlCommentLine to avoid naming conflict

23191b1

fixup getAPrimaryQlClass

ed54d7b

JS: remove duplicated predicate

b568bb9

Java: remove duplicated class

cc43a94

Java: add missing qldoc

bb32c79

JS: update expected test output

2569075

JS: add missing QLDoc

8f08b4c

RB: add missing QLDoc

c2787ee

add change-notes

1a275a3

JS: update expected output of test

4f8f7cd

erik-krogh force-pushed the acronyms branch from 5cccab6 to 4f8f7cd Compare March 11, 2022 10:18

erik-krogh mentioned this pull request Mar 11, 2022

Rename all upper-case variables, and all lower-case modules #8403

Merged

nickrolfe reviewed Mar 11, 2022

View reviewed changes

erik-krogh and others added 4 commits March 11, 2022 13:16

RB: add explicit-this

b63b675

Co-authored-by: Nick Rolfe <[email protected]>

JS: remove accidential copy-pasted change-note

a5a82a0

fix typo in change-note

2e29701

fix all other implicit-this warnings introduced by the acronym patch

1e36561

esbena approved these changes Mar 14, 2022

View reviewed changes

esbena mentioned this pull request Mar 14, 2022

JS: Address some code that weren't affecting any query result #8422

Merged

erik-krogh merged commit bbb2847 into github:main Mar 14, 2022

erik-krogh mentioned this pull request Mar 17, 2022

renaming more upper-case acronyms to PascalCase #8444

Closed

aschackmull added a commit to aschackmull/ql that referenced this pull request Mar 31, 2022

Java: Fix qldoc as followup to github#8323

f28da00

aschackmull mentioned this pull request Mar 31, 2022

Java: Fix acronym casing in qldoc referring to Java class names. #8625

Merged

erik-krogh mentioned this pull request Apr 6, 2022

Java: rename existing getUrl predicate to getRepositoryUrl #8679

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforcing consistent casing of acronyms #8323

Enforcing consistent casing of acronyms #8323

erik-krogh commented Mar 3, 2022 •

edited

Loading

tausbn Mar 3, 2022

igfoo Mar 4, 2022

erik-krogh Mar 4, 2022 •

edited

Loading

geoffw0 commented Mar 4, 2022

erik-krogh commented Mar 4, 2022 •

edited

Loading

geoffw0 commented Mar 7, 2022

hvitved left a comment

hvitved Mar 9, 2022

hvitved Mar 9, 2022

erik-krogh Mar 9, 2022

erik-krogh commented Mar 11, 2022

nickrolfe left a comment

aschackmull commented Mar 31, 2022

Enforcing consistent casing of acronyms #8323

Enforcing consistent casing of acronyms #8323

Conversation

erik-krogh commented Mar 3, 2022 • edited Loading

Conflicts detected from this:

The code in this PR:

Leftovers

tausbn Mar 3, 2022

Choose a reason for hiding this comment

igfoo Mar 4, 2022

Choose a reason for hiding this comment

erik-krogh Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

geoffw0 commented Mar 4, 2022

erik-krogh commented Mar 4, 2022 • edited Loading

geoffw0 commented Mar 7, 2022

hvitved left a comment

Choose a reason for hiding this comment

hvitved Mar 9, 2022

Choose a reason for hiding this comment

hvitved Mar 9, 2022

Choose a reason for hiding this comment

erik-krogh Mar 9, 2022

Choose a reason for hiding this comment

erik-krogh commented Mar 11, 2022

nickrolfe left a comment

Choose a reason for hiding this comment

aschackmull commented Mar 31, 2022

erik-krogh commented Mar 3, 2022 •

edited

Loading

erik-krogh Mar 4, 2022 •

edited

Loading

erik-krogh commented Mar 4, 2022 •

edited

Loading