Skip to content

Conversation

nickrolfe
Copy link
Contributor

@nickrolfe nickrolfe commented May 12, 2022

  1. Catch common misspellings as well.
  2. Also check names of classes, predicates, etc.

The old query matched against the entire QLDoc comment string, but now I break each comment string up into individual words (and do the same for node names). I was a bit worried about string pool pollution and performance, but they seem OK. We're talking about tens of thousands of strings for a db containing all of github/codeql, which is not that big a number.

@nickrolfe nickrolfe requested a review from a team as a code owner May 12, 2022 11:44
@nickrolfe nickrolfe marked this pull request as draft May 12, 2022 12:04
nickrolfe added 2 commits May 12, 2022 13:17
1. Catch common misspelling as well.
2. Also check names of classes, predicates, etc.
@nickrolfe nickrolfe force-pushed the nickrolfe/misspelling branch from e72cc9b to 6058352 Compare May 12, 2022 12:17
@nickrolfe
Copy link
Contributor Author

If @github/codeql-ql-for-ql-reviewers think this is ok, I'll add followup commits to fix some/many/all of the typos it finds.

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

If https://github.com/orgs/github/teams/codeql-ql-for-ql-reviewers think this is ok, I'll add followup commits to fix some/many/all of the typos it finds.

This looks good 👍

And there are plenty of typos to fix 🙈

@tausbn
Copy link
Contributor

tausbn commented May 12, 2022

Finally I won't be able to misspell "primordial" in my code. 💪

@nickrolfe
Copy link
Contributor Author

I've pushed fixes to all the spelling errors in QLDoc comments, and a couple of small refinements to the QL-for-QL query. I'm inclined to leave the remaining misspelled predicate and class names as they are for now.

@nickrolfe nickrolfe marked this pull request as ready for review May 12, 2022 15:13
@nickrolfe nickrolfe requested review from a team as code owners May 12, 2022 15:13
@nickrolfe nickrolfe requested review from a team as code owners May 12, 2022 15:13
@nickrolfe nickrolfe added the no-change-note-required This PR does not need a change note label May 12, 2022
Copy link
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++ 👍

@geoffw0 You might want to have a look at the XXE transformer misspellings flagged up here. I think those are coming from the query you're currently working on?

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Python changes LGTM. 👍

@geoffw0
Copy link
Contributor

geoffw0 commented May 12, 2022

@geoffw0 You might want to have a look at the XXE transformer misspellings flagged up here. I think those are coming from the query you're currently working on?

Looks like a genuine typo, in a QLDoc comment, that is unlikely to cause any merge conflicts with upcoming work.

So 👍 from me.

@nickrolfe
Copy link
Contributor Author

@geoffw0 - the updated query also flags a few class names in XXE.ql that misspell 'transformer' as 'tranformer'.

@geoffw0
Copy link
Contributor

geoffw0 commented May 12, 2022

@geoffw0 - the updated query also flags a few class names in XXE.ql that misspell 'transformer' as 'tranformer'.

Oh I see, there are some issues flagged but not fixed here. I can fix them in an upcoming PR?

Also: how did I not spot these myself...

@nickrolfe
Copy link
Contributor Author

Oh I see, there are some issues flagged but not fixed here. I can fix them in an upcoming PR?

Yeah, correcting spelling in comments is low-risk, so I just fixed them, but changing class and predicate names is a bit more involved, as I have to figure out whether I need to deprecate the old name and write a change-note.

So I've left that as a followup exercise, and I'd be more than happy for you to deal with those particular misspellings.

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 👍

@geoffw0
Copy link
Contributor

geoffw0 commented May 12, 2022

I'd be more than happy for you to deal with those particular misspellings.

Will do.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

C# / Ruby 👍

@nickrolfe nickrolfe merged commit c518150 into main May 16, 2022
@nickrolfe nickrolfe deleted the nickrolfe/misspelling branch May 16, 2022 15:03
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.

7 participants