refactor(review): extract the null-disables-label idiom into one helper (#4618) - #4756
Merged
Merged
Conversation
…er (#4618) 9 of the 13 near-identical per-repo "close/hold with this label" settings (blacklistLabel, contributorCapLabel, reviewNagLabel, reviewEvasionLabel, manualReviewLabel, readyToMergeLabel, changesRequestedLabel, migrationCollisionLabel, pendingClosureLabel) share the exact same "explicit null disables the label, undefined uses the operator-facing default" resolution idiom -- copy-pasted independently 9 times across settings/agent-actions.ts and queue/processors.ts rather than routed through a shared function. Adds resolveNullableLabel(configured, fallback) and replaces every call site with it. Pure mechanical DRY refactor -- zero behavior change, confirmed by the full existing test suite (which already exercised the null/undefined/explicit-string cases for each field) passing unchanged. This addresses the duplication half of #4618 item 3. The other half -- collapsing the 13 fields' TYPE surface, DB columns, and yml keys into one labels map -- is deliberately deferred: the fields are not type-uniform (3 different nullability shapes), only 6 of 13 have DB columns with a default, 2 have nullable columns with no default, 5 are config-as-code-only with no DB column at all, and the 13 keys are live top-level `.gittensory.yml` keys today, so a real collapse needs a backward-compat parsing shim and a data migration across 8 differently- shaped columns -- correctly scoped as its own follow-up per the issue's own "expect multiple PRs" framing.
4 tasks
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
4 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4756 +/- ##
==========================================
- Coverage 94.22% 94.22% -0.01%
==========================================
Files 441 441
Lines 38741 38737 -4
Branches 14117 14107 -10
==========================================
- Hits 36504 36500 -4
Misses 1577 1577
Partials 660 660
🚀 New features to boost your workflow:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Scoped-down slice of #4618 item 3, per discussion: the full type/DB/YAML collapse of the 13 label fields into one
labelsmap is deliberately deferred (see below) as a separate, higher-risk follow-up. This PR ships the safe, zero-behavior-change half.9 of the 13 near-identical per-repo "close/hold with this label" settings (
blacklistLabel,contributorCapLabel,reviewNagLabel,reviewEvasionLabel,manualReviewLabel,readyToMergeLabel,changesRequestedLabel,migrationCollisionLabel,pendingClosureLabel) share the exact same "explicitnulldisables the label,undefineduses the operator-facing default" resolution idiom — copy-pasted independently 9 times acrosssettings/agent-actions.tsandqueue/processors.tsrather than routed through a shared function.resolveNullableLabel(configured, fallback)and replaces every call site with it.Why the full collapse is deferred
Researched the complete footprint before deciding scope: the 13 fields are not type-uniform (3 different nullability shapes —
gittensorLabelis a bare requiredstring;newAccountLabel/moderationWarningLabel/moderationBannedLabelarestring | undefinedwith no null-disable semantics; the other 9 arestring | null | undefined). Only 6 of 13 have DB columns with a SQL default, 2 have nullable columns with no default, and 5 are config-as-code-only with no DB column at all. Critically, all 13 are live top-level.gittensory.ymlkeys today — a real collapse to a nestedlabels:map would break every self-hoster's existing config file unless a backward-compat parsing shim is added, on top of a data migration across 8 differently-shaped columns. That's correctly scoped as its own follow-up, matching the issue's own "expect multiple PRs" framing — not something to rush alongside 3 other PRs in one sitting given these labels gate real moderation/ban/close actions.Test plan
npx tsc --noEmit— cleannpm audit --audit-level=moderate— 0 vulnerabilitiesagent-actions.ts— nothing to mirror