Skip to content

[core] New RuleSet API and deprecations for PMD's entry point APIs#2635

Merged
adangel merged 29 commits into
pmd:masterfrom
oowekyala:ruleset-factory-builder
Dec 11, 2020
Merged

[core] New RuleSet API and deprecations for PMD's entry point APIs#2635
adangel merged 29 commits into
pmd:masterfrom
oowekyala:ruleset-factory-builder

Conversation

@oowekyala

@oowekyala oowekyala commented Jul 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Describe the PR

So this replaces RuleSetFactory/RuleSetFactoryUtils with a single RuleSetParser RuleSetLoader class. This replaces the complicated overloads of createFactory in RulesetFactoryUtils, and the constructors. This is more readable and lends itself to future extensions easily.

Background: I want to improve the error messages of RulesetFactory for PMD 7 (like, report something more helpful than an IllegalArgumentException stack trace), and those constructors/factory methods are too rigid to be modified.

RuleSetFactory can be internalized as the implementation of RuleSetParser (like RuleFactory is already), which is nice for the 7.0 schema update.

Also add new APIs where needed to make the entry point (PMD#processFiles) not rely on RuleSetFactory, nor on RuleSets. This is an ok transitional API I think.

Many other classes have been deprecated as internal, for example, AbstractPmdProcessor, SourceCodeProcessor, or PMDCommandLineInterface.

Ready?

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

@oowekyala oowekyala force-pushed the ruleset-factory-builder branch from 51e8a43 to 125b020 Compare October 26, 2020 19:35
@oowekyala oowekyala changed the title [core] Add ruleset factory builder [core] Add deprecations for PMD's entry point APIs Oct 26, 2020
@oowekyala oowekyala added this to the 6.30.0 milestone Oct 26, 2020
@oowekyala oowekyala added the is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them label Oct 26, 2020
@ghost

ghost commented Oct 27, 2020 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 This changeset introduces 0 new violations, 16 new errors and 0 new configuration errors,
removes 0 violations, 16 errors and 0 configuration errors.
Full report
✅ This changeset introduces 0 new violations, 16 new errors and 0 new configuration errors,
removes 0 violations, 16 errors and 0 configuration errors.
Full report
✅ This changeset introduces 0 new violations, 16 new errors and 0 new configuration errors,
removes 0 violations, 16 errors and 2 configuration errors.
Full report

Generated by 🚫 Danger

@adangel adangel changed the title [core] Add deprecations for PMD's entry point APIs [core] New RuleSet API and deprecations for PMD's entry point APIs Oct 30, 2020
adangel
adangel reviewed Oct 30, 2020
View reviewed changes
Comment thread docs/pages/release_notes.md Outdated
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/PMD.java Outdated
*
* @throws RuntimeException If processing fails
*/
public static void processFiles(final PMDConfiguration configuration,

@adangel adangel Oct 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we mark this as experimental? Or would we just deprecate this as well, if we have a better PMD programmatic API?

@oowekyala oowekyala Oct 30, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It may be possible to preserve compatibility with this signature in PMD 7. But also I think we should ideally keep a published entry point, that remains non-deprecated in the 6.x release cycle

Comment thread pmd-core/src/main/java/net/sourceforge/pmd/PMDException.java
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java Outdated
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/RuleSetParser.java Outdated
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/RuleSetParser.java Outdated
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/RuleSetParser.java Outdated
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/RuleSetParser.java Outdated
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/processor/AbstractPMDProcessor.java Outdated
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/processor/AbstractPMDProcessor.java
@adangel

adangel commented Oct 31, 2020

Copy link
Copy Markdown
Member

I've checked both maven-pmd-plugin and eclipse-pmd-plugin with these changes. They compile without changes, so there are no compatibility issues.

I've tried to resolve deprecations for m-pmd-p, which is not much: adangel/maven-pmd-plugin@008f771
There is one left: That's about RuleSetReferenceId, e.g. https://github.com/adangel/maven-pmd-plugin/blob/48956670777e7837fd3e37633c66f1a1d4f318e9/src/main/java/org/apache/maven/plugins/pmd/PmdReport.java#L441-L442
We use it there to parse the given ruleset, e.g. path/to/ruleset.xml/MyRule or http://download.example/ruleset.xml/MyRule. The plugin downloads the rulesets by itself, in order to place them in the target directory (https://maven.apache.org/plugins/maven-pmd-plugin/pmd-mojo.html#rulesetsTargetDirectory). Seems we need some kind of API to split such a "rulesetid" apart...

I didn't try to update eclipse-pmd-plugin to resolve deprecations, but the tests still run successful.

@oowekyala

oowekyala commented Nov 13, 2020

Copy link
Copy Markdown
Member Author

Should we keep RuleSetNotFoundException as part of the new API? It's very vague and is thrown for unrelated things, like IOExceptions (which are ignored as here). Besides, the ruleset parser also throws a lot of RuntimeExceptions, and any user of this API needs to handle all of them, and so is force to write catch (Exception e) (RuleSetNotFoundException is a checked exception)...

I believe it would be nicer to create an unchecked PmdXmlException and have RuleSetLoader only throw this one.

@oowekyala oowekyala marked this pull request as ready for review December 9, 2020 18:09
@adangel adangel self-assigned this Dec 11, 2020
adangel
adangel reviewed Dec 11, 2020
View reviewed changes
Comment thread pmd-core/src/main/java/net/sourceforge/pmd/RulesetLoadException.java
@adangel adangel mentioned this pull request Dec 11, 2020
Merged
adangel added a commit that referenced this pull request Dec 11, 2020
@adangel
Merge pull request #2635 from oowekyala:ruleset-factory-builder
1a4ca95 
[core] New RuleSet API and deprecations for PMD's entry point APIs #2635
@adangel adangel merged commit 0244ebf into pmd:master Dec 11, 2020
@adangel

adangel commented Dec 11, 2020

Copy link
Copy Markdown
Member

@oowekyala I'm trying to merge this now into pmd/7.0.x ....

@oowekyala oowekyala deleted the ruleset-factory-builder branch December 11, 2020 18:21
@oowekyala oowekyala mentioned this pull request Dec 12, 2020
Merged
4 tasks
@oowekyala oowekyala mentioned this pull request May 6, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adangel adangel adangel left review comments

Assignees

@adangel adangel

Labels

is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them

Projects

None yet

Milestone

6.30.0

Development

Successfully merging this pull request may close these issues.

2 participants

@oowekyala @adangel