Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW(pmd): @W-17605554@: Add tag based on ruleset name to standard rules that are referenced from custom ruleset #211

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

stephen-carter-at-sf
Copy link
Collaborator

No description provided.

@@ -212,7 +212,7 @@ void whenDescribeRulesIsGivenCustomRulesetThatContainsNameOfStandardRule_thenCus
PmdRuleInfo ruleInfo = assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "ApexCRUDViolation", "apex");
assertThat(ruleInfo.description, is("Sample ApexCRUDViolation description"));
assertThat(ruleInfo.externalInfoUrl, is("https://ApexCRUDViolation.com"));
assertThat(ruleInfo.ruleSet, is("sampleRuleset"));
assertThat(ruleInfo.ruleSets, is(List.of("sampleRuleset", "Security")));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is basically the reason for the JAVA changes... to keep tags for all rulesets that the rule was referenced from.

priority: string,
ruleSetFile: string,
class: string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't needed. It was left over from previous work and I forgot to remove it.

// Also test that when a bundled PMD rule is referenced inside a custom ruleset, that we update the rule
// with the tag associated with the custom ruleset's name.
const overriddenRuleDescription: RuleDescription = expectContainsRuleWithName(ruleDescriptions, 'DebugsShouldUseLoggingLevel'); // From somecat3.xml
expect(overriddenRuleDescription.tags).toEqual(['Recommended', 'BestPractices', 'Apex', 'SomeCat3']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the big win... we get both 'BestPractices' from the standard ruleset... and we get 'SomeCat3' from the custom ruleset.

Now users who specify custom ruleset files that also mention standard rules (or other custom rules) can run that entire ruleset using the tag based off of the ruleset name.

@@ -206,7 +211,7 @@ describe('Tests for the describeRules method of PmdEngine', () => {

const fakeRule1Description: RuleDescription = expectContainsRuleWithName(ruleDescriptions, 'fakerule1'); // From rulesets_apex_rules1.jar
expect(fakeRule1Description.severityLevel).toEqual(SeverityLevel.Moderate);
expect(fakeRule1Description.tags).toEqual(['Recommended', 'Categories1', 'Apex', 'Custom']);
expect(fakeRule1Description.tags).toEqual(['Recommended', 'Rulesets1', 'Apex', 'Custom']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes an issue where before it was referring to the parent ruleset instead of the explicitly provided ruleset.


// Add rule info
PmdRuleInfo pmdRuleInfo = new PmdRuleInfo();
pmdRuleInfo.name = rule.getName();
pmdRuleInfo.languageId = language;
pmdRuleInfo.description = getLimitedDescription(rule);
pmdRuleInfo.externalInfoUrl = rule.getExternalInfoUrl();
pmdRuleInfo.ruleSet = rule.getRuleSetName();
pmdRuleInfo.ruleSets.add(ruleSet.getName());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes an issue that I also discovered where it uses the top level ruleset name instead of the explicitely provided ruleset name.

For example if you have a jar containing 1 ruleset file A that references a rule from another ruleset file B within the same jar file... but the user only specifies ruleset file A... then we shouldn't tag the rule with B but with A. You'll see I updated a test to fix this on the typescript side.

@stephen-carter-at-sf stephen-carter-at-sf merged commit 6f0a838 into dev Feb 21, 2025
7 checks passed
@stephen-carter-at-sf stephen-carter-at-sf deleted the sc/W-17605554 branch February 21, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants