Skip to content

Shared: Overhaul the AlertFiltering QLDoc #20047

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Shared: Overhaul the AlertFiltering QLDoc #20047

wants to merge 2 commits into from

Conversation

cklin
Copy link
Contributor

@cklin cklin commented Jul 14, 2025

This PR strengthens the contract for the restrictAlertsTo and the restrictAlertsToExactLocation extensible predicates.

  • restrictAlertsTo is now documented to match any alert location that intersects with a specified line range. (Previously an alert location matches only when its first line is in a specified line range.)
  • restrictAlertsToExactLocation is now documented to match any alert location that wholly contains a specific character range. (Previously an alert location matchis only when it is exactly the same as a specified character range.)

It also contains misc wording changes for clarity.

@cklin
Copy link
Contributor Author

cklin commented Aug 11, 2025

@jbj @hvitved

This is a documentation update follow-up to #19943, concerning support for approximate locations. (That PR changes restrictAlertsTo diff-range location matching from "match the first line of location only" to "test whether the location intersects with the diffrange".)

#19943 treats support for approximate locations as an implementation detail that is not documented (nor expected to be documented.) This PR treats the same support for approximate locations as the new specification for restrictAlertsTo (and restrictAlertsToExactLocation), so that all other users of diff-informed queries, on both the QL authoring side and the CI integration side, can take advantage of this more general (and, I believe, more useful) behavior.

jbj
jbj previously approved these changes Aug 12, 2025
View reviewed changes
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

LGTM! It's great to see the documentation shrinking and becoming clearer while the use cases for diff-informed queries expand just by changing the spec to match the implementation.

Cc @asgerf.

@cklin cklin marked this pull request as ready for review August 12, 2025 14:39
@Copilot Copilot AI review requested due to automatic review settings August 12, 2025 14:39
@cklin cklin added the no-change-note-required This PR does not need a change note label Aug 12, 2025
Copilot
Copilot AI reviewed Aug 12, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the documentation for the AlertFiltering module to clarify and strengthen the contract for the restrictAlertsTo and restrictAlertsToExactLocation extensible predicates. The changes modify how alert location matching is defined - restrictAlertsTo now matches locations that intersect with a line range rather than just checking the first line, and restrictAlertsToExactLocation now matches locations that wholly contain a character range rather than requiring exact matches.

  • Updated parameter names and documentation for restrictAlertsTo to clarify line range intersection behavior
  • Modified restrictAlertsToExactLocation documentation to specify character range containment semantics
  • Simplified and clarified various docstring explanations throughout the module

@cklin
Shared: Overhaul the AlertFiltering QLDoc
b20521b 
This commit strengthens the contract for the restrictAlertsTo and the
restrictAlertsToExactLocation extensible predicates.

- restrictAlertsTo is now documented to match any alert location that
  intersects with a specified line range. (Previously an alert location
  matches only when its first line is in a specified line range.)

- restrictAlertsToExactLocation is now documented to match any alert
  location that wholly contains a specific character range. (Previously
  an alert location matchis only when it is exactly the same as a
  specified character range.)

It also contains misc wording changes for clarity.
@cklin cklin force-pushed the cklin/alert-filtering-qldoc branch from 7083d81 to b20521b Compare August 12, 2025 14:44
@cklin cklin requested a review from jbj August 12, 2025 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@jbj jbj jbj approved these changes

Assignees
No one assigned
Labels
no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cklin @jbj