Skip to content

[java] Deprecate visitor decorators#1437

Merged
adangel merged 20 commits into
pmd:masterfrom
oowekyala:deprecate-visitor-decorators
Nov 28, 2018
Merged

[java] Deprecate visitor decorators#1437
adangel merged 20 commits into
pmd:masterfrom
oowekyala:deprecate-visitor-decorators

Conversation

@oowekyala

@oowekyala oowekyala commented Nov 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Member
  • They're a pain to maintain, and are very much overengineered.
  • When we change parts of the grammar with 7.0.0, they're two additionnal duplications of JavaParserVisitorAdapter.
  • Removing them required a rewrite of the visitors for Cyclo and Ncss, but the tests didn't move, and it's probably a not-so-used part of PMD, so I think it's safe to say it's low-impact

Closes #1483

@oowekyala oowekyala added the in:metrics Affects the metrics framework label Nov 6, 2018
@oowekyala oowekyala added this to the 6.10.0 milestone Nov 6, 2018
@oowekyala oowekyala added the is:low-impact Used for changesets that can be reviewed very quickly, e.g. because they're not user-facing. label Nov 6, 2018
@ghost

ghost commented Nov 6, 2018 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 This changeset introduces 24 new violations and 0 new errors,
removes 27 violations and 0 errors. Full report
✅ This changeset introduces 24 new violations and 0 new errors,
removes 27 violations and 0 errors. Full report
✅ This changeset introduces 23 new violations and 0 new errors,
removes 27 violations and 0 errors. Full report

Generated by 🚫 Danger

@oowekyala oowekyala removed the is:low-impact Used for changesets that can be reviewed very quickly, e.g. because they're not user-facing. label Nov 6, 2018
@oowekyala
Find out reason for violation changes with Cyclo
0260062 
This is actually a bug fix.
Documented it on a test in CycloTest.xml
@oowekyala
Fix Ncss not counting lambda contents
7e5af86
@oowekyala oowekyala mentioned this pull request Nov 9, 2018
Merged
adangel
adangel reviewed Nov 20, 2018
View reviewed changes

@adangel adangel left a comment

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.

@oowekyala What do you think about the names in ASTIfStatements? Since we are introducing new methods now, we'll probably live with them for a while...

Do you have an overview of the AST nodes? the changes you made in this PR (constructors are internal api, javadoc) are probably needed for all nodes (and all languages)...

Comment thread pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTIfStatement.java Outdated
Comment thread pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/CycloVisitor.java
Comment thread pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/NcssVisitor.java
Comment thread pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/CycloTest.xml
Comment thread docs/pages/release_notes.md
@oowekyala

oowekyala commented Nov 25, 2018

Copy link
Copy Markdown
Member Author

TODO make the contents of the metrics.impl packages private API (deprecate, update doc)

@adangel adangel added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Nov 26, 2018
@oowekyala

oowekyala commented Nov 27, 2018

Copy link
Copy Markdown
Member Author

@adangel I tagged the contents of metrics.impl.visitors as internal API, but not the actual metrics in metrics.impl.Those e.g. declare MetricOptions in that package, and are documented there, so they're part of our API.

@oowekyala oowekyala removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Nov 28, 2018
@adangel adangel self-assigned this Nov 28, 2018
adangel
adangel approved these changes Nov 28, 2018
View reviewed changes

@adangel adangel left a comment

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.

Awesome, thanks!

@adangel adangel merged commit 47c66d4 into pmd:master Nov 28, 2018
@oowekyala oowekyala deleted the deprecate-visitor-decorators branch November 28, 2018 23:48
@oowekyala oowekyala mentioned this pull request Feb 17, 2019
Merged
@adangel adangel added the is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them label Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adangel adangel adangel approved these changes

Assignees

@adangel adangel

Labels

in:metrics Affects the metrics framework is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them

Projects

None yet

Milestone

6.10.0

Development

Successfully merging this pull request may close these issues.

[java] Cyclo metric should count conditions of for statements correctly

2 participants

@oowekyala @adangel