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.

Sign up for GitHub

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

Jump to bottom

NEW(cpd): @W-16866836@: Add java_command and rule_langage configurability to cpd engine #123

Merged
merged 4 commits into from
Nov 13, 2024
Merged

NEW(cpd): @W-16866836@: Add java_command and rule_langage configurability to cpd engine #123

merged 4 commits into from
Nov 13, 2024

Conversation

stephen-carter-at-sf
Copy link
Collaborator

@stephen-carter-at-sf stephen-carter-at-sf commented Nov 12, 2024 •
edited
Loading

In this review:

  • Added java_command and rule_languages fields to the cpd configuration.
  • The java code updates are to add in some better handling of exceptions that CPD could throw so that we can continue processing other languages even if one language fails.
  • Removed 'java' language support since John (our PM) agrees that we don't need it for CA5.
  • Made cpd rules 'Recommended' by default since it is super easy to turn them off - either disable engine, remove rule_languages, or just remove 'Recommended' tags from the cpd rules.

stephen-carter-at-sf
stephen-carter-at-sf commented Nov 12, 2024
View reviewed changes
...yzer-pmd-engine/pmd-cpd-wrappers/src/main/java/com/salesforce/sfca/cpdwrapper/CpdRunner.java

// Instead of throwing exceptions and causing the entire run to fail, instead we report exceptions as
// if they are processing errors so that they can better be handled on the typescript side
for (Exception ex : errorListener.exceptionsCaught) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed a bug in CPD and so I created pmd/pmd#5322. This bug actually prevented the engine from finishing across all languages since we just had java throw an exception which causes the engine to report the exception as a unexpected exception based violation.

But to improve the experience, so that CPD errors don't prevent us from finishing with other languages, I went ahead here and made the exceptions (messages + stack traces) show up as a special kind of processing error which then appears an ErrorEvent back to the CLI so that things continue and the error still gets reported.

This was a fix as you go item as I was self-QAing my own changes here.

packages/code-analyzer-pmd-engine/src/config.ts

export const PMD_ENGINE_CONFIG_DESCRIPTION: ConfigDescription = {
overview: getMessage('PmdConfigOverview'),
fieldDescriptions: {
java_command: getMessage('PmdConfigFieldDescription_java_command'),
rule_languages: getMessage('PmdConfigFieldDescription_rule_languages', PMD_AVAILABLE_LANGUAGES_TEXT),
java_command: getMessage('SharedConfigFieldDescription_java_command', PMD_ENGINE_NAME),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I first tried to use PmdEngine.NAME and CpdEngine.NAME but the compiler had issues with the order of things being loaded (since then config.ts would depend on pmd-engine.ts and vice versa).

So to fix this, I just moved the names of these engines into the constants.ts file.

packages/code-analyzer-pmd-engine/src/config.ts
@@ -98,11 +138,56 @@ class PmdConfigValueExtractor {
return javaCommand;
}

private async attemptToAutoDetectJavaCommand(): Promise<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.

Nothing new here... just moved this to a shared abstract class so that it could be reused for both cpd and pmd.

packages/code-analyzer-pmd-engine/src/cpd-engine.ts
@@ -124,7 +126,7 @@ function createRuleForLanguage(languageId: LanguageId): RuleDescription {
name: getRuleNameFromLanguage(languageId),
severityLevel: SeverityLevel.Info,
type: RuleType.MultiLocation,
tags: [`${languageId}Language`],
tags: ['Recommended', `${languageId}Language`],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: that we'll confirm this with John before going GA... but I am leaning towards Josh's preference here to just make these turned on by default with the 'Recommended' flag. Makes sense to me.

packages/code-analyzer-pmd-engine/test/plugin.test.ts
@@ -76,10 +87,10 @@ describe('Tests for the PmdCpdEnginesPlugin', () => {
}
});

it(`When createEngineConfig for 'pmd' is given an empty raw config and if our attempt to look-up java on the users machine produces old java version, then error`, async () => {
it.each(['cpd','pmd'])(`When createEngineConfig for '%s' is given an empty raw config and if our attempt to look-up java on the users machine produces old java version, then error`, async (engineName) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reusing many tests using it.each - since it is the Abstract class that has most of the logic behind what is being tested with the java_command and rule_languages fields for both pmd and cpd.

...code-analyzer-pmd-engine/test/test-data/cpdGoldfiles/rules_allDefaultLanguages.goldfile.json
"htmlLanguage"
],
"description": "Identify duplicate code blocks within your workspace files associated with the 'html' language.",
"resourceUrls": [
"https://docs.pmd-code.org/latest/pmd_userdocs_cpd.html#refactoring-duplicates"
]
},
{
"name": "DetectCopyPasteForJava",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We might remove java support alltogether (still waiting to confirm with John). But for now, although still available, it isn't one of the default languages anymore.

@stephen-carter-at-sf stephen-carter-at-sf merged commit a5588c6 into dev Nov 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfeingold35 jfeingold35 jfeingold35 approved these changes

@jag-j jag-j Awaiting requested review from jag-j

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stephen-carter-at-sf @jfeingold35