FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Move diff-range computation into utils by kaspersv · Pull Request #3238 · github/codeql-action · GitHub

  1. github
  2. codeql-action

Repository navigation

Move diff-range computation into utils - #3238

Merged
kaspersv merged 2 commits into
mainfrom
kaspersv/extract-diff-range-computation
Oct 27, 2025
ConversationCommits (2)ChecksFiles changed
Merged

Move diff-range computation into utils#3238
kaspersv merged 2 commits into
mainfrom
kaspersv/extract-diff-range-computation

Conversation

kaspersv commented Oct 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR moves the diff-range computations and associated tests into diff-informed-analysis-utils as preparation for a future PR where the diff-range computation will be performed during the init action instead of the analyze action. The PR is intended to be a pure refactoring, without any observable effects.

The two commits are derived from splitting the first commit from #3192.

Diff-informed analysis is guarded by the diff_informed_queries feature flag. The diff_informed_queries flag is already fully rolled out.

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

  • Advanced setup - Impacts users who have custom workflows.
  • Default setup - Impacts users who use default setup.
  • Code Scanning - Impacts Code Scanning (i.e. analysis-kinds: code-scanning).
  • Code Quality - Impacts Code Quality (i.e. analysis-kinds: code-quality).
  • GHES - Impacts GitHub Enterprise Server.

How did/will you validate this change?

  • Test repository - This change will be tested on a test repository before merging.
  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Feature flags - All new or changed code paths can be fully disabled with corresponding feature flags.
  • Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.
    • Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

github-actions Bot added the size/XL May be very hard to review label Oct 27, 2025
kaspersv changed the title Kaspersv/extract diff range computation Move diff-range computation into utils Oct 27, 2025
kaspersv requested a review from mbg October 27, 2025 11:44
kaspersv marked this pull request as ready for review October 27, 2025 11:44
kaspersv requested a review from a team as a code owner October 27, 2025 11:44
Copilot AI review requested due to automatic review settings October 27, 2025 11:44
Copilot AI reviewed Oct 27, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull Request Overview

This PR refactors diff-informed analysis functionality by moving diff-range computation logic and associated tests from analyze.ts to diff-informed-analysis-utils.ts. The changes prepare for future work where diff-range computation will occur during the init action instead of the analyze action.

  • Moved getPullRequestEditedDiffRanges, getFileDiffsWithBasehead, getDiffRanges functions and FileDiff interface from analyze.ts to diff-informed-analysis-utils.ts
  • Relocated corresponding test cases from analyze.test.ts to diff-informed-analysis-utils.test.ts
  • Updated imports and exports to reflect the new module structure

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/diff-informed-analysis-utils.ts Added diff-range computation functions and FileDiff interface previously in analyze.ts
src/diff-informed-analysis-utils.test.ts Added test cases for getDiffRanges function moved from analyze.test.ts
src/analyze.ts Removed diff-range computation functions, updated imports to use new exports from diff-informed-analysis-utils
src/analyze.test.ts Removed getDiffRanges test cases now in diff-informed-analysis-utils.test.ts
lib/analyze-action.js Generated JavaScript reflecting the TypeScript changes

mbg approved these changes Oct 27, 2025
View reviewed changes

mbg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This change looks OK. I am wondering if it would be better to move this to somewhere like api-client.ts though, since there isn't really anything specific to diff-informed queries.

Comment thread src/diff-informed-analysis-utils.ts
// uses forward slashes as the path separator, so on Windows we need to
// replace any backslashes with forward slashes.
const filename = path
.join(actionsUtil.getRequiredInput("checkout_path"), fileDiff.filename)

mbg Oct 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I note that this input is specific to the analyze and upload-sarif actions, so this will need some kind of change if you are planning to call it from the init action in a follow-up change.

kaspersv Oct 27, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the pointer. I will keep that in mind for the next changes.

kaspersv commented Oct 27, 2025

Copy link
Copy Markdown
Contributor Author

I am wondering if it would be better to move this to somewhere like api-client.ts though, since there isn't really anything specific to diff-informed queries.

I think an argument can be made both ways. It is somewhat specific to diff-informed analysis in that getDiffRanges (and consequently getPullRequestEditedDiffRanges) is designed to return the information needed specifically for diff-informed analysis rather than all the information returned by compareCommitsWithBasehead. In particular, diff-informed analysis currently works at a file-level granularity and therefore the functions don't return any column information even though it could be extracted.

I am happy to move the functions to api-client.ts if you prefer.

mbg commented Oct 27, 2025

Copy link
Copy Markdown
Member

No strong feelings. I am happy that some parts are specific to diff-informed analysis and, while there might be parts we could extract for more general usage, we can always do that if/when we actually need them for something else. Feel free to merge this as-is.

kaspersv merged commit 8d77149 into main Oct 27, 2025
kaspersv deleted the kaspersv/extract-diff-range-computation branch October 27, 2025 14:40
This was referenced Oct 30, 2025
Merged
Merged
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
mbg mbg approved these changes

Assignees

No one assigned

Labels

size/XL May be very hard to review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL