Add query to ensure predicates starting with 'get' return a value #18164
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Napalys
force-pushed
the
napalys/ql-validate-predicate-get-returns
branch
from
85c922f to
a763dd7
Compare
aschackmull
reviewed
Nov 29, 2024
aschackmull
reviewed
Nov 29, 2024
Co-authored-by: Anders Schack-Mulligen <[email protected]>
Napalys
force-pushed
the
napalys/ql-validate-predicate-get-returns
branch
from
5fa40c4 to
a5521b9
Compare
Co-authored-by: Anders Schack-Mulligen <[email protected]>
aschackmull
reviewed
Nov 29, 2024
| isGetPredicate(pred) and | ||
| not hasReturnType(pred) and | ||
| not isAlias(pred) | ||
| select pred, "This predicate starts with 'get' but does not return a value." |
There was a problem hiding this comment.
This message is slightly inaccurate now 😄
aschackmull
reviewed
Nov 29, 2024
ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected
Outdated
Show resolved
Hide resolved
Napalys
force-pushed
the
napalys/ql-validate-predicate-get-returns
branch
from
1f3187b to
a462ec9
Compare
Napalys
force-pushed
the
napalys/ql-validate-predicate-get-returns
branch
from
9b9581f to
a908a44
Compare
… thought they had
Napalys
force-pushed
the
napalys/ql-validate-predicate-get-returns
branch
from
a908a44 to
7c1aa84
Compare
aschackmull
reviewed
Dec 2, 2024
| /** | ||
| * Returns "get" if the predicate name starts with "get", otherwise "as". | ||
| */ | ||
| string getPrefix(Predicate pred) { |
There was a problem hiding this comment.
There's really no need for this predicate - just replace regexpMatch with regexpCapture in isGetPredicate and add the prefix column to that predicate.
There was a problem hiding this comment.
I see, that does make it cleaner!
67745e6
hvitved
reviewed
Dec 2, 2024
| where | ||
| isGetPredicate(pred, prefix) and | ||
| not hasReturnType(pred) and | ||
| not isAlias(pred) |
There was a problem hiding this comment.
Should we instead check that whatever it aliases has a return type?
There was a problem hiding this comment.
I think that's a very reasonable extension of this query, but I don't mind merging as-is, hence approved.
There was a problem hiding this comment.
Is this what you have had in mind 01b62ad ?
aschackmull
previously approved these changes
Dec 2, 2024
hvitved
requested changes
Dec 2, 2024
…ates lacking a return type. Co-authored-by: asgerf <[email protected]>
Napalys
force-pushed
the
napalys/ql-validate-predicate-get-returns
branch
from
01b62ad to
7db9b7d
Compare
hvitved
approved these changes
Dec 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new query to ensure that predicates starting with 'get' actually return a value. It includes the query definition, expected test results, and test cases to validate the functionality.