AlertFiltering: add restrictAlertsToExactLocation #18603
Conversation
This commit rephrases the documentation for the restrictAlertsTo predicate and renames the predicate columns for clarity. The new documentation should be equivalent to the old documentation, except allowing for the possibility that there may be multiple alert filtering predicates.
cklin
force-pushed
the
cklin/restrict-alerts-to-exact
branch
from
de8805e to
1d39110
Compare
|
I am not expecting any performance change from this PR, in the common case where I ran some local performance tests with
The evaluation times are close enough to support the case that performance of diff-informed queries remain unchanged. I also verified that alert filtering works properly for both |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
shared/util/ext/default-alert-filter.yml:11
- The newly introduced extension 'restrictAlertsToExactLocation' lacks explicit test coverage. Please add or update tests to verify it functions as expected.
extensible: restrictAlertsToExactLocation
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
| * Note that an alert that is not accepted by this filtering predicate may still be included in the | ||
| * query results if it is accepted by another active filtering predicate in this module. An alert is | ||
| * excluded from the query results if only if (1) there is at least one active filtering predicate, | ||
| * and (2) it is not accepted by any active filtering predicate. |
There was a problem hiding this comment.
The duplication of these paragraphs between the two sets of QLDoc feels a bit awkward. Is there a way to have it appear only once, e.g. in a comment for the whole file?
If that is not possible, it might still be better to explicitly separate the general rules from the ones that are specific to the filtering predicate in question. As it is, each predicate has two general paragrams ("Note that an alert ..." as well as "A query should either perform ...") interlaced with specific information.
There was a problem hiding this comment.
My main goal is to ensure that the qldoc for each filtering predicate provides all relevant information for the behavior and the use of that predicate, as I consider the predicate qldoc to be the only piece of documentation that everyone can be reasonably expected to read. That is why some of the same text appears in both qldocs.
When seeing both qldocs together in the source code, I understand that the duplication feels redundant and messy, especially when common information is interleaved with specific information. That said, qldoc can appear outside the context of the QL source code, and one qldoc may be shown without the other. I want to optimize for the ease of understanding for each qldoc by itself, even at the risk of being overly verbose.
Which is not to say that the qldocs are above criticism—I am sure there is a lot of room for improvement, and I am happy to hear your suggestions. Though I will also assess any suggestions using the criteria outlined above, unless someone makes a good case that those are not the goals to optimize for.
There was a problem hiding this comment.
When I write a long piece of duplicated QLDoc like that, I like to end it with "See also: [names of the predicates with similar QLDoc]". Then future maintainers are also alerted to the coupling.
There was a problem hiding this comment.
When I write a long piece of duplicated QLDoc like that, I like to end it with "See also: [names of the predicates with similar QLDoc]". Then future maintainers are also alerted to the coupling.
Thanks. I updated the PR to add the cross references.
This commit introduces a new extensible predicate restrictAlertsToExactLocation, which is similar to the existing restrictAlertsTo predicate but matches alert locations exactly.
cklin
force-pushed
the
cklin/restrict-alerts-to-exact
branch
from
1d39110 to
96caa68
Compare
This commit introduces a new extensible predicate
restrictAlertsToExactLocation, which is similar to the existingrestrictAlertsTopredicate but matches alert locations exactly.restrictAlertsToExactLocationis intended for situations such as testing, where we know the exact alert location and want to distinguish between alert locations that are potentially on the same line.