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

[kotlin] [cpd] Added CPD support for Kotlin #1441

Merged
merged 9 commits into from Nov 26, 2018

Conversation

Projects
None yet
7 participants
@maikelsteneker
Copy link
Contributor

maikelsteneker commented Nov 7, 2018

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 is a pull request for the addition of Kotlin support for CPD. This does not include full PMD support, so it can only be used to check for code duplication.

See #419 and #551.

}
}

private boolean isDiscarding() {

This comment has been minimized.

@jsotuyod

jsotuyod Nov 8, 2018

Member

all filtering / discarding should be done through a net.sourceforge.pmd.cpd.token.TokenFilter, as is done in many other languages.

Even 'though we don't currently have it in place for Antlr (yet!), on JavaCC we support // CPD-OFF and // CPD-ON comment-based suppression for all JavaCC languages with a single implementation this way

Show resolved Hide resolved pmd-kotlin/pom.xml Outdated
@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 8, 2018

@maikelsteneker thanks for the PR, this is going to make a lot of people happy!

If you could provide a sample kotlin source file and include a test case it would be great.

Once again, the pmd-go module is a nice example as to what we would expect for unit testing a new cpd-only language (version and tokenizer test).

Thanks in advanced!

@maikelsteneker

This comment has been minimized.

Copy link
Contributor

maikelsteneker commented Nov 8, 2018

Thanks for the comments! I'll get back to them and make some adjustments later.

@maikelsteneker

This comment has been minimized.

Copy link
Contributor

maikelsteneker commented Nov 12, 2018

I've taken most of your comments into account. Unfortunately, I wasn't able to use a TokenFilter, since the AntlrTokenizer doesn't seem to support it. I think adjusting it falls outside of the scope of this change, so I've worked around it for now.

Please let me know if additional changes should be made before merging. Thanks again for the feedback!

@pmd-test

This comment has been minimized.

Copy link

pmd-test commented Nov 12, 2018

1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

/**
* Language Module for Kotlin
*/
public class KotlinLanguageModule extends BaseLanguageModule {

This comment has been minimized.

@oowekyala

oowekyala Nov 12, 2018

Member

@jsotuyod @adangel Actually, why do we need a PMD Language service for CPD-only languages? They're just dummy implementations but all CPD-only modules have them. They have no use since LanguageRegistry filters them out (with findWithRuleSupport), and they could be sources of bugs because the only thing that gives away that they're unsupported is that they return some nulls and throw some UnsupportedOperationExceptions... Should we end this practice?

This comment has been minimized.

@adangel

adangel Nov 20, 2018

Member

Yes, the PMD Module is not required in that case... and we should not add it then....

@adangel adangel added this to the 6.10.0 milestone Nov 25, 2018

@adangel adangel self-assigned this Nov 25, 2018

@adangel adangel merged commit 166e17f into pmd:master Nov 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

adangel added a commit that referenced this pull request Nov 26, 2018

@adangel

This comment has been minimized.

Copy link
Member

adangel commented Nov 26, 2018

Thanks for your contribution! This will be part of PMD 6.10.0. 🎉

@maikelsteneker

This comment has been minimized.

Copy link
Contributor

maikelsteneker commented Nov 26, 2018

Cool, thanks a lot! I'm looking forward to the new release.

@JLLeitschuh

This comment has been minimized.

Copy link

JLLeitschuh commented Jan 3, 2019

Wow. This is huge! Nice job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment