QL: add ql/consistent-alert-message #10053
Conversation
| any(TopLevel top | top.getLocation().getFile() = file).getQLDoc() = doc | ||
| | | ||
| result = | ||
| doc.getContents().regexpCapture("(?s).*@id (\\w+)/([\\w\\-]+)\\s.*", 2) // query ID (without lang) |
There was a problem hiding this comment.
The duplication of this regex here and on line 54 is a bit unfortunate. Perhaps it would be nicer to factor these out in a helper predicate on the QLDoc type?
| string getMessage(Select sel) { | ||
| result = | ||
| strictconcat(String e, Location l | | ||
| // is child of an expression in the select (in an uneven position, that's where the message is) |
There was a problem hiding this comment.
... in an uneven position ...
That's an odd way to phrase that. 😉
(Though I'm not sure "odd position" is actually more intuitive. Maybe "odd-indexed position" would be better?)
There was a problem hiding this comment.
Though I'm not sure "odd position" is actually more intuitive.
That's not an odd position to have 😉
I'll go for odd-indexed position.
| string getSelectFingerPrint(Select sel) { | ||
| exists(File file, QLDoc doc | | ||
| sel.getLocation().getFile() = file and | ||
| any(TopLevel top | top.getLocation().getFile() = file).getQLDoc() = doc |
There was a problem hiding this comment.
I feel like this could be convenient to push into a getQLDoc predicate on File.
There was a problem hiding this comment.
I've made a class QueryDoc, and it felt more appropriate to have an accessor for that on Select.
| * The query-id (without language), the language, the message from the select, and a language agnostic fingerprint. | ||
| */ | ||
| Select parseSelect(string id, string lang, string msg, string fingerPrint) { | ||
| exists(File file, QLDoc doc | result.getLocation().getFile() = file | |
There was a problem hiding this comment.
Suggestion: why not create a subclass of QLDoc for the kind of QLDoc that applies to a whole file, and then expose the relevant tags as member predicates?
(I think it's fine to only implement the predicates you need for this query. We can always add more later, and some of them may be a bit fiddly to implement.)
| // for a select with a fingerprint | ||
| sel = parseSelect(id, lang, msg, fingerPrint) and | ||
| // there exists other languages with the same fingerprint, but other message | ||
| badLangs = |
There was a problem hiding this comment.
The way the alert message is formulated, it sounds like lang is bad compared to everything in badLangs, but then wouldn't it make more sense to call this goodLangs?
Or perhaps just otherLangs to be more neutral.
(In the case where lang is the only outlier, it seems strange to classify the other languages as bad.)
| * Gets a language agnostic fingerprint for a Select. | ||
| * The fingerPrint includes e.g. the query-id, the @kind of the query, and the number of expressions in the select. | ||
| * | ||
| * This fingerprint avoid false positives where two queries with the same ID behave differently (which is OK). |
There was a problem hiding this comment.
| * This fingerprint avoid false positives where two queries with the same ID behave differently (which is OK). | |
| * This fingerprint avoids false positives where two queries with the same ID behave differently (which is OK). |
And drive-by remove some alerts on deprecated predicates/classes.
I got a WIP branch were I've fixed most of the alerts introduced by the new query.