Skip to content

QL: improve the "this block-comment should have been a QLDoc"-query #11294

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
Nov 23, 2022

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Nov 16, 2022

This would have caught a recent QLDoc check that I spent way too long trying to fix, where I didn't realise that the top-level comment was not a QLDoc.

I also fixed a whole bunch of the issues found by the query.
Not all, some results are benign.

And also some improvements to how we match up QLDoc in QL-for-QL.

@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Nov 16, 2022
@erik-krogh erik-krogh marked this pull request as ready for review November 16, 2022 20:33
@erik-krogh erik-krogh requested a review from a team as a code owner November 16, 2022 20:33
@erik-krogh erik-krogh requested a review from a team November 16, 2022 20:33
@erik-krogh erik-krogh requested review from a team as code owners November 16, 2022 20:33
Copy link
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

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

C++ differences LGTM.

node instanceof TopLevel and
comment = getCommentAtStart(node.getLocation().getFile(), 1) and
nodeDescrip = "the file"
) and
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we could remove some redundant logic here by adding a reason parameter to getACommentThatCouldBeQLDoc returning "the below code" or "the file".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that could save some duplication.
But that sounds like something for a followup, this seems good enough to merge.

@erik-krogh erik-krogh merged commit 1eec067 into github:main Nov 23, 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.

3 participants