Skip to content

Document regular expressions in sync_back.ts#3547

Open
mbg wants to merge 1 commit intomainfrom
mbg/ts/sync-back/feedback
Open

Document regular expressions in sync_back.ts#3547
mbg wants to merge 1 commit intomainfrom
mbg/ts/sync-back/feedback

Conversation

@mbg
Copy link
Member

@mbg mbg commented Mar 5, 2026

Follow up to #3529 to address @esbena's review comment.

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?

Environments:

  • Testing/None - This change does not impact any CodeQL workflows in production.

How did/will you validate this change?

  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
  • End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

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

  • Development/testing only - This change cannot cause any failures in production.

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

  • Other - Please provide details.

Are there any special considerations for merging or releasing this change?

  • No special considerations - This change can be merged at any time.

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.

@mbg mbg requested a review from esbena March 5, 2026 15:25
@mbg mbg self-assigned this Mar 5, 2026
@mbg mbg requested a review from a team as a code owner March 5, 2026 15:25
Copilot AI review requested due to automatic review settings March 5, 2026 15:25
@github-actions github-actions bot added the size/S Should be easy to review label Mar 5, 2026
Copy link
Contributor

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 improves the readability/maintainability of the pr-checks/sync_back.ts sync-back script by documenting and centralizing the regular expressions it uses to detect and update GitHub Action references.

Changes:

  • Adds detailed documentation and names for the regexes used to extract and replace action references.
  • Refactors regex construction/replacement into reusable SyncBackPattern builders plus a shared escaping helper.
Comments suppressed due to low confidence (2)

pr-checks/sync_back.ts:77

  • The comment for YAML_PATTERN says it groups uses: actionName and rest_of_line, but the regex only captures uses: actionName (group 1) and does not capture the remainder of the line. Please correct the comment (or add a second capture group if that was the intent).
/**
 * Used to find lines containing action references in a PR check specification.
 *
 * Matches `uses: actionName@rest_of_line` in PR check specifications and groups `uses: actionName`
 * and `rest_of_line`, allowing `rest_of_line` to be replaced with a new version string.
 */
const YAML_PATTERN: SyncBackPattern = (actionName: string) =>
  new RegExp(`(uses:\\s+${actionName})@(?:[^@\n]+)`, "g");

pr-checks/sync_back.ts:52

  • ESCAPE_PATTERN is used to escape regex metacharacters before interpolating actionName into a RegExp, not for escaping TypeScript/YAML string literals. Consider tweaking this comment to avoid implying it handles string escaping.
/**
 * Used to identify characters in `action_name` strings that need to
 * be escaped before inserting them into TypeScript or YAML strings.
 */
const ESCAPE_PATTERN = /[.*+?^${}()|[\]\\]/g;

@mbg mbg force-pushed the mbg/ts/sync-back/feedback branch from aaf2d8d to 6531f80 Compare March 5, 2026 16:51
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

Thank you. This matches my earlier request, and it probably would have gone through if it had been done during that pull request. But since I reviewed it in isolation now, I suppose I'm a bit more strict in terms of how we can do this parting.

* - `09a7688d3b55cf0e976497ff046b70949eeaccfd # v1.288.0`.
*/
const EXTRACT_ACTION_REF_PATTERN: RegExp =
/uses:\s+([^/\s]+\/[^@\s]+)@([^@\n]+)/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe anchor it such that we expect it to be at the start of the line?

Suggested change
/uses:\s+([^/\s]+\/[^@\s]+)@([^@\n]+)/g;
/^\s*uses:\s+([^/\s]+\/[^@\s]+)@([^@\n]+)/g;

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it's probably a minor thing, but in terms of YAML semantics, the user could have added quotes around the right-hand side. Meaning that the captured content will contain surrounding quotes.

I'm thinking we could improve robustness against this if we use the current regular expression to find interesting lines, and then reparse them as standalone YAML. We can then take the right-hand side value instead of the right-hand side syntactic definition.

* allowing `rest_of_line` to be replaced with a new version string.
*/
const YAML_PATTERN: SyncBackPattern = (actionName: string) =>
new RegExp(`(uses:\\s+${actionName})@(?:[^@\n]+)`, "g");
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, we can probably expect this to be the start of the trimmed line.

Suggested change
new RegExp(`(uses:\\s+${actionName})@(?:[^@\n]+)`, "g");
new RegExp(`^\s*(uses:\\s+${actionName})@(?:[^@\n]+)`, "g");

@mbg
Copy link
Member Author

mbg commented Mar 7, 2026

Thank you. This matches my earlier request, and it probably would have gone through if it had been done during that pull request. But since I reviewed it in isolation now, I suppose I'm a bit more strict in terms of how we can do this parting.

I ended up questioning why we are using regular expressions here at all and ended up preparing an alternative to this at #3556 which avoids using regular expressions here entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Should be easy to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants