Skip to content

make acronyms camelcase#10096

Merged
erik-krogh merged 8 commits into
github:mainfrom
erik-krogh:acronyms-part1
Aug 24, 2022
Merged

make acronyms camelcase#10096
erik-krogh merged 8 commits into
github:mainfrom
erik-krogh:acronyms-part1

Conversation

@erik-krogh
Copy link
Copy Markdown
Contributor

@erik-krogh erik-krogh commented Aug 18, 2022

Acronyms should use normal PascalCase/camelCase.

This is a followup to #10073, and there might be more later.

Most of this PR was written automatically by my patching tool.

A QLDoc check was complaining about some missing QLDoc, so I fixed that (even though I didn't introduce the code 🤷).

Copy link
Copy Markdown

@github-advanced-security github-advanced-security AI left a comment

Choose a reason for hiding this comment

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

Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Copy Markdown

@github-advanced-security github-advanced-security AI left a comment

Choose a reason for hiding this comment

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

Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.

@github-actions github-actions Bot added the Swift label Aug 18, 2022
class CamelToUri extends string {
CamelToUri() {
exists(SpringCamelXmlToElement toXmlElement | this = toXmlElement.getUri()) or
exists(CamelJavaDSLToDecl toJavaDSL | this = toJavaDSL.getUri())

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase.

Acronyms in toJavaDSL should be PascalCase/camelCase
@erik-krogh erik-krogh force-pushed the acronyms-part1 branch 3 times, most recently from f2a8456 to 62e8575 Compare August 18, 2022 13:47
@github-actions github-actions Bot removed the Swift label Aug 18, 2022
@erik-krogh erik-krogh force-pushed the acronyms-part1 branch 3 times, most recently from 3ead1c4 to be7aec4 Compare August 18, 2022 20:17
@erik-krogh erik-krogh force-pushed the acronyms-part1 branch 3 times, most recently from be8982c to a1244ec Compare August 22, 2022 19:08
Copy link
Copy Markdown

@github-advanced-security github-advanced-security AI left a comment

Choose a reason for hiding this comment

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

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Copy Markdown

@github-advanced-security github-advanced-security AI left a comment

Choose a reason for hiding this comment

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

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

@erik-krogh erik-krogh marked this pull request as ready for review August 23, 2022 07:48
@erik-krogh erik-krogh requested review from a team as code owners August 23, 2022 07:48
Comment thread java/ql/lib/semmle/code/xml/XML.qll Outdated
Comment on lines 180 to 184
/** DEPRECATED: Alias for XmlDtd */
deprecated class XmlDTD = XmlDtd;

/** DEPRECATED: Alias for XmlDtd */
deprecated class XMLDTD = XmlDTD;
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull Aug 23, 2022

Choose a reason for hiding this comment

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

This was just introduced in your prior PR, so I think we can combine these aliases.

Suggested change
/** DEPRECATED: Alias for XmlDtd */
deprecated class XmlDTD = XmlDtd;
/** DEPRECATED: Alias for XmlDtd */
deprecated class XMLDTD = XmlDTD;
/** DEPRECATED: Alias for XmlDtd */
deprecated class XMLDTD = XmlDtd;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same for C++.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same for C++.

And the other languages, I'm fixing all in one go.

michaelnebel
michaelnebel previously approved these changes Aug 23, 2022
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

C# LGTM

Comment on lines +101 to +102
/** DEPRECATED: Alias for SslServerSocket */
deprecated class SSLServerSocket = SslServerSocket;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we need a deprecated alias in the experimental dir.

Copy link
Copy Markdown
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

A couple of comments, otherwise Java LGTM.

@smowton
Copy link
Copy Markdown
Contributor

smowton commented Aug 23, 2022

In Go I notice a couple of cases where a newtype constructor doesn't get a deprecated alias (though the type name itself does), though perhaps that's too technically tricky to recreate?

@smowton
Copy link
Copy Markdown
Contributor

smowton commented Aug 23, 2022

Otherwise Go LGTM

Copy link
Copy Markdown
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.

Python 👍 besides the requested changes

Comment thread python/ql/lib/semmle/python/frameworks/Stdlib.qll Outdated
Comment thread python/ql/src/Security/CWE-327/PyOpenSSL.qll Outdated
Comment thread python/ql/src/Security/CWE-327/PyOpenSSL.qll Outdated
Comment thread python/ql/src/Security/CWE-327/PyOpenSSL.qll Outdated
Comment thread python/ql/src/Security/CWE-327/Ssl.qll Outdated
Comment thread python/ql/src/Security/CWE-327/Ssl.qll Outdated
Comment thread python/ql/src/Security/CWE-327/Ssl.qll Outdated
Comment thread python/ql/src/Security/CWE-327/Ssl.qll Outdated
@erik-krogh
Copy link
Copy Markdown
Contributor Author

erik-krogh commented Aug 23, 2022

In Go I notice a couple of cases where a newtype constructor doesn't get a deprecated alias (though the type name itself does), though perhaps that's too technically tricky to recreate?

@smowton: It's because it's private. It's not accessible from the outside, so it doesn't need a deprecated alias.

smowton
smowton previously approved these changes Aug 23, 2022
Copy link
Copy Markdown
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Yeah ok on closer inspection the deprecated alias was for a nearby entity not the newtype itself

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@erik-krogh erik-krogh dismissed stale reviews from smowton and michaelnebel via 7704a9e August 23, 2022 08:38
Copy link
Copy Markdown
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

C++ 👍

@erik-krogh erik-krogh requested a review from RasmusWL August 23, 2022 08:42
Copy link
Copy Markdown
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.

Python 👍

Copy link
Copy Markdown
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Java 👍

Comment thread cpp/ql/lib/semmle/code/cpp/XML.qll
Copy link
Copy Markdown
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Ruby 👍

Copy link
Copy Markdown

@kaeluka kaeluka left a comment

Choose a reason for hiding this comment

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

JS: 👍 LGTM. One comment is that the change note(s) make it sound as if this would be breaking a lot of customer code, when the old spelling is actually still supported in the form of a deprecated alias. Edit: Erik pointed out that the change note mentions that, I just missed it because I focused on only the diff :)

Comment thread javascript/ql/lib/change-notes/2022-08-22-xml-rename.md
@erik-krogh erik-krogh merged commit 4df2e5d into github:main Aug 24, 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.

10 participants