CHANGE @W-22462048@ apex guru tags update - #489
Conversation
|
Git2Gus App is installed but the |
aruntyagiTutu
left a comment
There was a problem hiding this comment.
This PR silently removes customer-facing tags from every ApexGuru rule and deletes the tests that guarded that behavior, with no PR description explaining why.
Issues:
-
No PR description. Per the PR Hygiene guideline, the description must explain the change and match the code. This PR body is completely empty, so there's no rationale for why
COMMON_TAGS.CATEGORIES.PERFORMANCE/BEST_PRACTICESandCOMMON_TAGS.LANGUAGES.APEXare being stripped from all ~24 rules inapexguru-rules.ts. These tags appear to drive customer-facing rule filtering (e.g.--rule-selector), so removing them is a behavior change, not a cosmetic one. -
Tests deleted instead of updated. The second commit ("test case fix") removes the
COMMON_TAGSimport and the fourit/it.eachblocks inapexguru-rules.test.tsthat specifically asserted every rule retains its category and language tags:'%s should retain its category tag''every rule should have a category tag (Performance or BestPractices)''every rule should have Apex language tag'
These are exactly the tests that would fail against commit 1's change. Deleting them removes the regression signal rather than updating the assertions to reflect an intentional new tag contract. If the tag removal is deliberate, the tests should be rewritten to assert the new expected tag set, not deleted outright.
Please add a PR description explaining the motivation for dropping these tags (e.g., is ApexGuru moving away from category/language-based filtering? Is this tied to a broader tagging redesign?), and restore test coverage that asserts the new intended tag contract rather than removing coverage entirely.
aruntyagiTutu
left a comment
There was a problem hiding this comment.
Thanks for the follow-up — this fully addresses the earlier concerns.
- PR description now clearly explains the motivation:
DevPreviewApexGuruis meant to be an opt-in-only tag so ApexGuru rules aren't reachable via broad selectors (--rule-selector Performance,--rule-selector 4,all) during the dev preview phase. - The previously-deleted test coverage wasn't just restored — it was replaced with more targeted coverage:
apexguru-rules.test.ts/ApexGuruEngine.test.tsupdated to the new tag, plus a newDevPreviewApexGuru rule selection behaviorsuite inrule-selection.test.ts(backed by a newDevPreviewEnginePluginstub) that explicitly asserts the opt-in-only selection semantics. rules.ts#matchesRuleSelectornow actually implements that semantics (excludingall/severity from selectables fordevpreviewapexguru-tagged rules), with an inline comment explaining why — good that the behavior matches what the new tests assert rather than just renaming a constant.
Approving.
On the tag removal: The motivation was to introduce a new tag DevPreviewApexGuru as the sole tag on ApexGuru rules. This is tied to a broader change in how ApexGuru rules participate in rule selection — the intent is that ApexGuru rules should only be invocable via explicit opt-in (engine name, rule name, or the DevPreviewApexGuru tag), not via broad selectors like --rule-selector 4 or --rule-selector Performance. Removing COMMON_TAGS.CATEGORIES.* and COMMON_TAGS.LANGUAGES.APEX from these rules is deliberate: those tags would otherwise make ApexGuru rules selectable by category/language selectors in ways we don't want during the dev preview phase.