Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Antlr visitor rules #1774

Merged
merged 9 commits into from
Jun 15, 2019

Conversation

lsoncini
Copy link
Contributor

This pull request depends on #1698.

Before submitting a PR, please check that:

  • The PR is submitted against master. The PMD team will merge back to support branches as needed.
  • ./mvnw clean verify passes. This will build and test PMD, execute PMD and checkstyle rules. Check this for more info

PR Description:

This pull request adds the necessary classes to implement antlr-based abstract rules and generic implementations of both RuleChainVisitor (AntlrRuleChainVisitor) and RuleViolationFactory (AntlrRuleViolationFactory).

We also added AbstractSwiftRule to serve as an example to show how to implement AbstractAntlrVisitor for a specific language and modified the ant-file antlr4.xml to make antlr's autogenerated BaseVisitors extend AbstractAntlrVisitor.

Team Raptor.

@pmd-test
Copy link

pmd-test commented Apr 13, 2019

1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

@oowekyala oowekyala self-requested a review April 15, 2019 15:41
Copy link
Member

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

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

This is an interesting change, it introduces some nice ideas that I think we can take further to reduce the complexity of language implementations (outside of this PR)

@jsotuyod jsotuyod changed the title Feature/antlr visitor rules [core] Antlr visitor rules Apr 17, 2019
@jsotuyod jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Apr 22, 2019
@jsotuyod jsotuyod added this to the 7.0.0 milestone Apr 22, 2019
Copy link
Member

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

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

@matifraga I think, the comments I made can be handled at a later stage, as they indeed are not limited to Antlr rules. I'll open a couple of tickets to track them. This PR seems otherwise perfectly fine to me and I think it can be merged.

@adangel adangel self-assigned this Jun 15, 2019
@adangel adangel added an:enhancement An improvement on existing features / rules and removed is:WIP For PRs that are not fully ready, or issues that are actively being tackled labels Jun 15, 2019
@adangel adangel merged commit 5307f8a into pmd:pmd/7.0.x Jun 15, 2019
adangel added a commit that referenced this pull request Jun 15, 2019
@adangel
Copy link
Member

adangel commented Jun 15, 2019

@lsoncini @matifraga @tomidelucca
I've merged now your PRs to PMD 7. I don't know, if anything works, since I didn't see a unit test 😃

I added the ANTLR support on the wiki here: https://github.com/pmd/pmd/wiki/PMD-7.0.0#language-specific-improvements to track the progress a bit and see, what's still ahead.

Here are the points I see next:

  • Create a sample rule for swift
  • XPath support for Antlr based grammars
  • Unit tests for Antlr support
  • Documentation

@matifraga matifraga deleted the feature/antlr-visitor-rules branch June 15, 2019 15:19
@matifraga
Copy link
Contributor

Thanks @adangel ! Sorry about unit testing, we will unit test the classes we introduced already. We do have a working rule for Swift already, we will send a PR soon.

Regarding XPath our first attempt making a rule didn't work out, we are currently digging the code to find out why. We will also document with examples all that is needed to add a new rule using antlr as we did with the CPD feature.

cc/ @lsoncini @tomidelucca @jsotuyod

@adangel
Copy link
Member

adangel commented Jun 15, 2019

Don't worry. I just wanted to make sure, you guys have this on your radar...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
an:enhancement An improvement on existing features / rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants