[kotlin] Kotlin support#3505
Conversation
…d language" except $. Generate your Parser did not succeed because combining lexer and parser grammars has issues.
…ammar; and move PmdKotlinParser.java to the right place
Generated by 🚫 Danger |
|
@jborgers Could you add a couple of rules and their tests to verify that everything works properly? Would also be nice to see parser tests, see eg SwiftParserTests |
…lasses.g4 file, added <libDirectory> to maven antlr plugin to make it work
… with unit test cases
…for custom kotlin xpath functions
…sts with pmd/7.0.x branch
…found by pmd-sonar plugin
non-abstract abstract classes These are abstract classes without abstract methods. They can be made final without problems.
pmd7-ClassWithOnlyPrivateConstructorsShouldBeFinal-abstract
…d language" except $. Generate your Parser did not succeed because combining lexer and parser grammars has issues.
…ammar; and move PmdKotlinParser.java to the right place
…lasses.g4 file, added <libDirectory> to maven antlr plugin to make it work
… with unit test cases
…for custom kotlin xpath functions
…found by pmd-sonar plugin
…T in sonatype snapshots repo
|
Hi, letting you know we are still working on this PR. oowekyala: Seems that a rebase made this PR a bit messy. Is that the case or are we still good? Bit confused what is best approach to merge upstream. Furthermore, we managed to run pmd-7 with kotlin within sonar-pmd locally, so that is also promising. See https://github.com/jborgers/sonar-pmd/tree/pmd-7 |
|
@oowekyala @adangel Could you please help us with the above question. Can we merge this PR?
|
|
I'll have a look at the next few days.... |
| <groupId>net.sourceforge.pmd</groupId> | ||
| <artifactId>pmd</artifactId> | ||
| <version>7.0.0-SNAPSHOT</version> | ||
| <version>7.0.0-kotlin-SNAPSHOT</version> |
There was a problem hiding this comment.
I was surprised to finally find the Kotlin module in this PR... Please fix the history, I appreciate the effort but this is too much of a mess. And why would you change the version number?
There was a problem hiding this comment.
Thanks for your reply. The mess is because of the rebase. Peter Paul was not sure how to manage it, therefore his question. The version change was to deal with a difficulty we faced. He can better explain that himself and we should find a solution for it with you. Unfortunately he is on vacation right now, but he will try to reply tonight anyway.
There was a problem hiding this comment.
Ok well I see no particular urgency, if he's on vacation we can let that be until later
There was a problem hiding this comment.
About the version number: when locally building and trying out pmd 7.0.0-SNAPSHOT with the Kotlin module from our branch, such as in the pmd-designer and the sonar-pmd plugin, we run into weird issues. We would use a local pmd-7.0.0-SNAPSHOT, but it seems when a new one is released in Sonatype, newer than our own local build (e.g. https://oss.sonatype.org/service/local/repositories/snapshots/content/net/sourceforge/pmd/pmd/7.0.0-SNAPSHOT/pmd-7.0.0-20220219.093841-581.pom), builds would use that one (without the Kotlin module), and things would break unexpectedly. By using another version, these version clashes would not occur.
Because we work on the 7.0.x branch in a fork I thought is would be a good idea to rebase our Kotlin changes on top of the recent 7.0.x branch, but it seems it does not work as I expected and it messes with the commit history.
What is the best way to make a clean PR? Maybe we can make a new branch in our fork on the most recent 7.0.x branch and cherry-pick our Kotlin commits in that one.
(and leave out the 7.0.0-kotlin-SNAPSHOT version and just use that for our local test builds, or find another way to avoid these version clashes)
There was a problem hiding this comment.
Thanks for replying!
Do you still have a ref to the branch as it was before you rebased (working)? That would be the easiest, just git reset --hard to there, and merge the newest 7.0.x instead of rebasing on it. Then force push.
If you don't then I think checking out the latest 7.0.x, then checking out the pmd-kotlin module files off the broken branch, then fixing the remaining compilation issues one by one. Cherry-picking commits is probably harder than git checkout.
When we merge this PR, the auto-released snapshots will contain your work with the version 7.0.0-SNAPSHOT. We can't merge a changeset that also changes the version of that snapshot, even if I understand it is helpful locally. Maybe that will fix your issue with sonar though
adangel
left a comment
There was a problem hiding this comment.
Thanks for your work on Kotlin!
In the current state, this PR can neither be reviewed nor merged.
It contains irrelevant commits (which belong to branch pmd/7.0.x)
and it contains the relevant commits twice.
I took the liberty to create a new PR (#3808) - the original commits where
still there. I only reverted the version change (this would break
everything for us) and then merged pmd/7.0.x into the branch.
I hope, this will make it easier, to see only the relevant change
for reviewing.
As @oowekyala said, once this PR is merged, it should make it easier
for you to use the "official" 7.0.0-SNAPSHOT version. However,
you will still see the official builds taking over, when you don't
build locally - that's the nature of SNAPSHOTS - always the latest
will be used.
You could maybe avoid this, if you run dependent builds in offline
mode (./mvnw --offline). Then newer SNAPSHOTS from sonatype
should not be downloaded and only the local SNAPSHOTS should be used.
Regarding the merge: IMHO I'm ok merging it in this state. It
basically seems to be working. However, I'm not sure, we should keep
the AST as it is. For example, "package foo.bar;" is parsed into
individual tokens/nodes (T-package, T-identifier, T-dot, T-identifier)
which would need to be collected together into a single node.
But that's not something that needs to be done immediately. We
maybe should mark this language as experimental until we know
how it should look like.
|
I'm closing this now in favor of #3808 @jborgers @stokpop - since I opened the new PR, you won't have write access to it. So, please comment there or point me to your changes in your fork, if you want to have more changes integrated. Otherwise, it would be easier, if you create a new PR for further changes, after #3808 is merged. |
|
Thanks @adangel! I will try the Maven offline mode. I read to start working offline, first to download all needed deps using: |
Describe the PR
Implements the steps of "Adding PMD support for a new ANTLR grammar based language" for Kotlin
Related issues
Ready?
No, work in progress
./mvnw clean verifypasses (checked automatically by github actions)