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.

Sign up for GitHub

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

Jump to bottom

[swift] Feature/swift rules #1877

Merged
merged 21 commits into from
Aug 13, 2019
Merged

[swift] Feature/swift rules #1877

merged 21 commits into from
Aug 13, 2019

Conversation

matifraga
Copy link

@matifraga matifraga commented Jun 20, 2019 •
edited
Loading

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 PR adds an XPath and a visitor rule for the Swift language.

oowekyala
oowekyala reviewed Jun 20, 2019
View reviewed changes
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.

Not a thorough review, just a couple of comments. Keep up the good work

pmd-test/src/main/java/net/sourceforge/pmd/testframework/RuleTestRunner.java
@@ -123,23 +116,24 @@ public void evaluate() throws Throwable {
return statement;
}

private Statement withBefores(Statement statement) {
List<FrameworkMethod> befores = getTestClass().getAnnotatedMethods(Before.class);
private Statement withBefores(final Statement statement) {
Copy link
Member

Choose a reason for hiding this comment

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

Note to maintainers: We should really make it clear where we expect final modifiers on local vars & parameters in our codebase. I think it's just noise in short methods, especially when it pollutes a diff like that.

Copy link
Author

Choose a reason for hiding this comment

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

There is a PMD rule that make finals required :P It's not enabled on this project though.

@ghost
Copy link

ghost commented Jun 21, 2019

1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

@matifraga matifraga marked this pull request as ready for review June 21, 2019 02:09
@tomidelucca tomidelucca mentioned this pull request Jun 21, 2019
Merged
2 tasks
@adangel adangel changed the title [Swift] Feature/swift rules [swift] Feature/swift rules Jun 30, 2019
@adangel adangel added this to the 7.0.0 milestone Jun 30, 2019
@oowekyala oowekyala self-requested a review July 22, 2019 11:13
@matifraga
Copy link
Author

@jsotuyod Thanks for the review! we will add guidance for developers regarding the rules we added, and provide the examples. Regarding the ClassCastException antlr terminal nodes, are different entities than the rest of the nodes, so if you try to access one using the rule, you will retrieve an empty node. We now it's not the cleanest way to handle this, but a proper rule should never encounter this scenario, because each leaf node has a context with the information regarding the terminal node. We think this a good trade off given the amount of work that would require handling terminal nodes properly, and in doing so we don't have anything to gain.

@jsotuyod
Copy link
Member

@tomidelucca @matifraga I took the liberty of refactoring how you handled terminal nodes.

I also turned the rule to a rulechain rule and found rule chain indexation was not working, so had to fix that. Please review yourselves before I merge.

@oowekyala I added a comment on AbstractRule#addRuleChainVisit(Class<? extends Node> nodeClass), as the implementation is far from ideal… this was a preexisting issue, so I'm not handling it on this PR, but we may want to think of a better way to get the XPath name of a node given the class name.

@matifraga
Copy link
Author

Nice catch!

@jsotuyod I'm ok with the changes, we weren't aware of the rule chain related code on AbstractRule, sorry we missed that! I'm also ok with skipping the terminal nodes and throwing an exception instead of silencing it.

@jsotuyod jsotuyod merged commit 53eba55 into pmd:pmd/7.0.x Aug 13, 2019
jsotuyod added a commit that referenced this pull request Aug 13, 2019
@jsotuyod
Update changelog, refs #1877
af72ddd
@jsotuyod jsotuyod added a:new-language Proposal to add a new PMD or CPD language to the main distribution a:new-rule Proposal to add a new built-in rule labels Aug 14, 2019
@adangel adangel mentioned this pull request Jan 23, 2023
Closed
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsotuyod jsotuyod jsotuyod left review comments

@oowekyala oowekyala Awaiting requested review from oowekyala

Assignees
No one assigned
Labels
a:new-language Proposal to add a new PMD or CPD language to the main distribution a:new-rule Proposal to add a new built-in rule
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@matifraga @jsotuyod @oowekyala @adangel @lsoncini