Document regular expressions in sync_back.ts - #3547
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
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.
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;
Sorry, something went wrong.
redsun82
left a comment
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
esbena
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| * - `09a7688d3b55cf0e976497ff046b70949eeaccfd # v1.288.0`. | ||
| */ | ||
| const EXTRACT_ACTION_REF_PATTERN: RegExp = | ||
| /uses:\s+([^/\s]+\/[^@\s]+)@([^@\n]+)/g; |
There was a problem hiding this comment.
Should we maybe anchor it such that we expect it to be at the start of the line?
| /uses:\s+([^/\s]+\/[^@\s]+)@([^@\n]+)/g; | |
| /^\s*uses:\s+([^/\s]+\/[^@\s]+)@([^@\n]+)/g; |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| * 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"); |
There was a problem hiding this comment.
As above, we can probably expect this to be the start of the trimmed line.
| new RegExp(`(uses:\\s+${actionName})@(?:[^@\n]+)`, "g"); | |
| new RegExp(`^\s*(uses:\\s+${actionName})@(?:[^@\n]+)`, "g"); |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I will close this in favour of #3556 |
Sorry, something went wrong.
Follow up to #3529 to address @esbena's review comment.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist