NEW (Extension) @W-13991634@ Add class level PMD violations #91

jag-j · 2024-06-05T00:59:49Z

This PR does the following:

Injects PMD's class-level suppression syntax to silence a single rule for that class (i.e., @SuppressWarnings syntax).
Identifies the specific rule to be suppressed and adds the appropriate syntax (Example: @SuppressWarnings('PMD.ApexDoc')).
If a class-level suppression tag already exists, it changes the SuppressWarnings accordingly (Example: @SuppressWarnings('PMD.ApexDoc', 'PMD.UnusedMethod')), instead of adding a new tag.
Adds the tag for Apex and Java appropriately (double-quotes for Java).
Identifies the correct position to add this tag by regex parsing the file for 'class', but at the same type ignoring 'class' defined as a comment or as part of a string.

jag-j · 2024-06-05T01:29:35Z

src/lib/fixer.ts

+
+        action.diagnostics = [this.diagnostic];
+        action.command = {
+            command: Constants.COMMAND_REMOVE_DIAGNOSTICS_ON_SELECTED_FILE,


FYI, I clear the diagnostics for the whole file and this is not ideal when a file has multiple classes. Keeping track of the diagnostics that would be affected by the rule added on the specific class seemed too difficult and the AC for the story did not call for it. Since this PR is already doing too many things, I could still do it as part of a separate story.

Clarification: Does "multiple classes" mean multiple outer classes, or an outer class with inner classes? I know Apex can't have multiple outer classes, and I'm pretty sure that the annotation will only apply to outer classes, though I haven't checked that.

Didn't realize apex can't have multiple outer classes. I have changed the code and now this applies only to the outer class.

stephen-carter-at-sf · 2024-06-05T13:16:09Z

src/lib/fixer.ts

+    public generateClassLevelSuppression(): vscode.CodeAction {
+        // Find the end-of-line position of the class declaration where the diagnostic is found.
+        const classStartPosition = this.findClassStartPosition(this.diagnostic, this.document);
+        // const classEndOfLinePosition = 0;


Please remove commented out code.

stephen-carter-at-sf · 2024-06-05T13:25:08Z

src/lib/fixer.ts

+            const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim());
+            if (!existingRules.includes(suppressionRule)) {


Just wondering about case sensitivity of the rules. If they are case insensitive, then we probably should add to the map a .toLowerCase() and then add it to the suppressionRule when checking for existence.

Follow-up question: Is it an error case if the rule is already present, or is it valid and nothing should happen?

I have added case insensitivity in the regex and added tests. It is an error case when a rule is already present and violations are present. One scenario I can think of is when user ran the scanner without the rule and manually added the rule and then tried to run the suppress at the class level. I'm not handling that part.

Adding case insensitivity in the regex doesn't help with this line does it existingRules.includes(suppressionRule)) ?

I'd recommend something like
const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim().toLowerCase());
if (!existingRules.includes(suppressionRule.toLowerCase())) {

As discussed offline, this has an unintended side effect. It changes all existing rules to lower case since we concatenate the existing rules (which were made to be lower case for comparison) to new rules. So, we decided to leave this as is.

stephen-carter-at-sf · 2024-06-05T13:30:22Z

src/lib/fixer.ts

+    /**
+     * Finds the start position of the class in the document.
+     * Assumes that the class declaration starts with the keyword "class".
+     * @returns The position at the start of the class.
+     */
+    public findClassStartPosition(diagnostic: vscode.Diagnostic, document: vscode.TextDocument): vscode.Position {


I was confused at first by this... I didn't realize until I read the code that you always return column 0. This sounds like it is the position (line, col) exactly where you find the word "class". But in reality you are finding the starting line of the class and returning the zeroth column. I recommend you update your comment to make this clear. Otherwise, I was scratching my head earlier in the code wondering how you could put a newline character immediately before the word "class".

src/lib/fixer.ts

stephen-carter-at-sf · 2024-06-05T13:35:59Z

src/lib/fixer.ts

+    public isWithinQuotes(line: string, matchIndex: number): boolean {
+        const beforeMatch = line.slice(0, matchIndex);
+        const singleQuotesBefore = (beforeMatch.match(/'/g) || []).length;
+        const doubleQuotesBefore = (beforeMatch.match(/"/g) || []).length;


Does apex allow double quotes? I thought strings must be single quoted? Not sure if you need to worry about double quotes.

Strings have to be single-quoted. I genuinely don't remember if you're allowed to use double-quotes as string boundaries.
BUT you might want to check whether the string containing escaped single-quotes throws off the algorithm. E.g., 'this is a string. it\'s got a single-quote inside it'.

stephen-carter-at-sf · 2024-06-05T13:37:56Z

src/lib/fixer.ts

+        // Check if the number of quotes before the match is odd (inside quotes)
+        if (singleQuotesBefore % 2 !== 0 || doubleQuotesBefore % 2 !== 0) return true;
+
+        return false;


lower golf score:
return <conditional_statement>

jfeingold35 · 2024-06-05T14:19:46Z

src/lib/fixer.ts

+
+        // Determine the appropriate suppression rule based on the type of diagnostic.code
+        let suppressionRule: string;
+        if (typeof this.diagnostic.code === 'object' && this.diagnostic.code !== null && 'value' in this.diagnostic.code) {


Do you need both the === 'object' and !== null check? Seems like the first would be sufficient. Also, in JS/TS it's typical to do == null instead of === null, because undefined == null and usually you want to catch both null and undefined.

jfeingold35 · 2024-06-05T14:21:18Z

src/lib/fixer.ts

+            const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim());
+            if (!existingRules.includes(suppressionRule)) {


Follow-up question: Is it an error case if the rule is already present, or is it valid and nothing should happen?

jfeingold35 · 2024-06-05T14:22:24Z

src/lib/fixer.ts

+
+        action.diagnostics = [this.diagnostic];
+        action.command = {
+            command: Constants.COMMAND_REMOVE_DIAGNOSTICS_ON_SELECTED_FILE,


Clarification: Does "multiple classes" mean multiple outer classes, or an outer class with inner classes? I know Apex can't have multiple outer classes, and I'm pretty sure that the annotation will only apply to outer classes, though I haven't checked that.

src/lib/fixer.ts

jfeingold35 · 2024-06-05T14:29:14Z

src/lib/fixer.ts

+    public isWithinQuotes(line: string, matchIndex: number): boolean {
+        const beforeMatch = line.slice(0, matchIndex);
+        const singleQuotesBefore = (beforeMatch.match(/'/g) || []).length;
+        const doubleQuotesBefore = (beforeMatch.match(/"/g) || []).length;


Strings have to be single-quoted. I genuinely don't remember if you're allowed to use double-quotes as string boundaries.
BUT you might want to check whether the string containing escaped single-quotes throws off the algorithm. E.g., 'this is a string. it\'s got a single-quote inside it'.

jfeingold35 · 2024-06-05T14:30:08Z

src/lib/fixer.ts

+        const doubleQuotesBefore = (beforeMatch.match(/"/g) || []).length;
+
+        // Check if the number of quotes before the match is odd (inside quotes)
+        if (singleQuotesBefore % 2 !== 0 || doubleQuotesBefore % 2 !== 0) return true;


Pet peeve: I'm broadly opposed to single-line/braceless ifs.

stephen-carter-at-sf · 2024-06-10T17:38:38Z

src/lib/fixer.ts

+            const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim());
+            if (!existingRules.includes(suppressionRule)) {


Adding case insensitivity in the regex doesn't help with this line does it existingRules.includes(suppressionRule)) ?

I'd recommend something like
const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim().toLowerCase());
if (!existingRules.includes(suppressionRule.toLowerCase())) {

jag-j added 6 commits June 4, 2024 09:52

NEW (Extension) @W-13991634@ Add class level PMD violations

22f52fd

NEW (Extension) @W-13991634@ Add class level PMD violations - part 2

a5ff50b

NEW (Extension) @W-13991634@ Add class level PMD violations - part 3

d21b16b

NEW (Extension) @W-13991634@ Add class level PMD violations - part 4

980c7b4

NEW (Extension) @W-13991634@ Add class level PMD violations - part 5

3a5a424

NEW (Extension) @W-13991634@ Add class level PMD violations - part 6

729dfb9

jag-j requested review from jfeingold35 and stephen-carter-at-sf June 5, 2024 01:00

jag-j commented Jun 5, 2024

View reviewed changes

stephen-carter-at-sf reviewed Jun 5, 2024

View reviewed changes

jfeingold35 reviewed Jun 5, 2024

View reviewed changes

jag-j added 4 commits June 7, 2024 09:29

Merge branch 'dev' into jj/W-13991634

8d40ccb

fix failing test due to merge

ab56d58

address code review comments

b7a2ddb

address code review comments - 2

40f5faf

stephen-carter-at-sf approved these changes Jun 10, 2024

View reviewed changes

jag-j merged commit 54363ff into dev Jun 10, 2024
7 checks passed

jag-j deleted the jj/W-13991634 branch June 10, 2024 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW (Extension) @W-13991634@ Add class level PMD violations #91

NEW (Extension) @W-13991634@ Add class level PMD violations #91

jag-j commented Jun 5, 2024 •

edited

Loading

jag-j Jun 5, 2024

jfeingold35 Jun 5, 2024

jag-j Jun 10, 2024

stephen-carter-at-sf Jun 5, 2024

stephen-carter-at-sf Jun 5, 2024

jfeingold35 Jun 5, 2024

jag-j Jun 10, 2024

stephen-carter-at-sf Jun 10, 2024

jag-j Jun 10, 2024

stephen-carter-at-sf Jun 5, 2024

stephen-carter-at-sf Jun 5, 2024

jfeingold35 Jun 5, 2024

stephen-carter-at-sf Jun 5, 2024

jag-j Jun 10, 2024

jfeingold35 Jun 5, 2024

jag-j Jun 10, 2024

jfeingold35 Jun 5, 2024

jfeingold35 Jun 5, 2024

jfeingold35 Jun 5, 2024

jfeingold35 Jun 5, 2024

stephen-carter-at-sf Jun 10, 2024

		const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim());
		if (!existingRules.includes(suppressionRule)) {

NEW (Extension) @W-13991634@ Add class level PMD violations #91

NEW (Extension) @W-13991634@ Add class level PMD violations #91

Conversation

jag-j commented Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jag-j commented Jun 5, 2024 •

edited

Loading