Skip to content

[apex] New Rule: Queueable Should Attach Finalizer#5303

Merged
adangel merged 9 commits intopmd:mainfrom
mitchspano:Require_Finalizer
Nov 17, 2024
Merged

[apex] New Rule: Queueable Should Attach Finalizer#5303
adangel merged 9 commits intopmd:mainfrom
mitchspano:Require_Finalizer

Conversation

@mitchspano
Copy link
Copy Markdown
Contributor

@mitchspano mitchspano commented Nov 1, 2024

Describe the PR

Warns the author of Apex whenever they write a class which implements the Queueable interface and do not attach a Finalizer.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@mitchspano mitchspano changed the title Require finalizer Queueable Should Attach Finalizer Nov 1, 2024
@adangel adangel changed the title Queueable Should Attach Finalizer [apex] Queueable Should Attach Finalizer Nov 4, 2024
@adangel adangel changed the title [apex] Queueable Should Attach Finalizer [apex] New Rule: Queueable Should Attach Finalizer Nov 4, 2024
Copy link
Copy Markdown
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I think, the rule documentation should be improved a bit. I'm suggesting a slightly different (more neutral) name. We have also some more guidelines documented at https://docs.pmd-code.org/latest/pmd_devdocs_major_rule_guidelines.html

The goal is, that the rules are easily understandable, what they detect, why they detect it, and what the alternative ("correct") code would be...

Comment thread pmd-apex/src/main/resources/category/apex/bestpractices.xml
Comment thread pmd-apex/src/main/resources/category/apex/bestpractices.xml Outdated
Comment thread pmd-apex/src/main/resources/category/apex/bestpractices.xml Outdated
Comment thread pmd-apex/src/main/resources/category/apex/bestpractices.xml
mitchspano added 2 commits November 12, 2024 19:46
- Renames the rule to `QueueableWithoutFinalizer` to be more neutral.
- Provides a more robust description.
- Provides a more succinct error message.
- Provides a positive sample for the documentation .
Copy link
Copy Markdown
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

However, this is not ready yet: There are some PMD violations, you need to fix first. And then, we'll probably see checkstyle errors (we use 4 space indentation, etc. - just run ./mvnw verify before commit.... see also https://github.com/pmd/pmd/wiki/Setup-IDE)

@adangel adangel added the a:new-rule Proposal to add a new built-in rule label Nov 14, 2024
@mitchspano mitchspano requested a review from adangel November 15, 2024 21:56
@mitchspano
Copy link
Copy Markdown
Contributor Author

@all-contributors please add @mitchspano for code

@mitchspano
Copy link
Copy Markdown
Contributor Author

@adangel Thank you for your guidance here. I have resolved all PMD and checkstyle issues. ./mvnw verify produces a passing result.

@adangel adangel added this to the 7.8.0 milestone Nov 17, 2024
@ghost
Copy link
Copy Markdown

ghost commented Nov 17, 2024

1 Message
📖 Compared to main:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 11 errors and 7 configuration errors.
Download full report as build artifact

Generated by 🚫 Danger

Copy link
Copy Markdown
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

adangel added a commit that referenced this pull request Nov 17, 2024
adangel added a commit that referenced this pull request Nov 17, 2024
Merge pull request #5303 from mitchspano:Require_Finalizer
@adangel adangel merged commit 85aeebb into pmd:main Nov 17, 2024
@mitchspano mitchspano deleted the Require_Finalizer branch December 29, 2024 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:new-rule Proposal to add a new built-in rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[apex] New Rule: Queueable Should Attach Finalizer

2 participants