Skip to content

Conversation

@stephen-carter-at-sf
Copy link
Contributor

@stephen-carter-at-sf stephen-carter-at-sf commented Mar 18, 2024

  • Remove PMD6 all together
  • Use PMD 7.0.0 release now as default (updating inclusion list as well with new dependencies)
  • Remove --preview-pmd7 flag
  • Remove warnings about incompatible rules for PMD 7 (since we are now in PMD 7)
  • Temporarily turn off unit test for pmd-appexchange (which will come in a change immediately following this one)
  • Update smoke tests and sample code to reflect PMD 7

it('Case: --engine pmd-appexchange against unclean code', () => {
// Currently this test fails because the pmd-appexchange jar files depend on classes that only exist in PMD6
// TODO: Turn this test back on as soon as we get the new jar files for pmd-appexchange that work with PMD7
xit('Case: --engine pmd-appexchange against unclean code', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will reenable this with PR: #1411 ... but this PR needs to be merged first.

@stephen-carter-at-sf stephen-carter-at-sf changed the title [DRAFT] Make PMD7 the new default and remove PMD6 CHANGE (CodeAnalyzer): @W-13887486@: Make PMD7 the new default and remove PMD6 Mar 22, 2024
@stephen-carter-at-sf stephen-carter-at-sf marked this pull request as ready for review March 22, 2024 14:36
"lint-typescript": "eslint ./src --ext .ts --max-warnings 0",
"test": "./gradlew test jacocoTestCoverageVerification && nyc mocha --timeout 10000 --retries 5 \"./test/**/*.test.ts\"",
"test-quiet": "cross-env SFGE_LOGGING=false ./gradlew test jacocoTestCoverageVerification && nyc mocha --timeout 10000 --retries 5 \"./test/**/*.test.ts\"",
"test": "./gradlew test jacocoTestCoverageVerification && nyc mocha --timeout 60000 --retries 5 \"./test/**/*.test.ts\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we making this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You'll notice that test-typescript already has a --timeout of 60000 ... so I was updating the others to reflect that change in all the commands. Basically on a given users machine a few tests go over 10 seconds... on my machine 3 tests consistently take 12 to 13 seconds. In the past, Josh updated test-typescript (which is the command he uses locally) to 60 seconds. I personally use yarn test locally to just run all the tests for all subprojects. So increasing this timeout to 60 seconds is the safest thing to do... it allows us to run things locally without tests dying but gives a sanity check for a run away test to stop and fail in our build system.

extractRules();

// STEP 2: Process the category files to derive category and rule representations.
// STEP: Process the category files to derive category and rule representations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Remove "STEP:" so the comments are consistent.

Copy link
Contributor Author

@stephen-carter-at-sf stephen-carter-at-sf Mar 22, 2024

Choose a reason for hiding this comment

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

ok - good catch. done.

Copy link
Contributor

@jfeingold35 jfeingold35 left a comment

Choose a reason for hiding this comment

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

Looks good, basically just the one stylistic change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file has some tabs-vs-spaces conflicts. If the new changes can be tabs, then it'll all be fine.

Copy link
Contributor Author

@stephen-carter-at-sf stephen-carter-at-sf Mar 22, 2024

Choose a reason for hiding this comment

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

ok - done.

Comment on lines 25 to 26
<groupId>net.sourceforge.pmd</groupId>
<artifactId>pmd-apex</artifactId>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested this locally and built it successfully. The only dependency needed is the pmd-apex v7.0.0 now.

@stephen-carter-at-sf
Copy link
Contributor Author

Fixes #1293

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.

[Feature Request] Upgrade Apex Parser to support new Apex features such as the null coalesce operator

4 participants