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

[WIP] Add ANTLR4 support #328

Merged
merged 42 commits into from
Jul 1, 2020
Merged

Conversation

matthiasbalke
Copy link
Contributor

@matthiasbalke matthiasbalke commented Dec 28, 2018

This PR adds support for formatting ANTLR4 grammars using Antlr4Formatter and Gradle.

Checklist:

  • add formatter step to lib
  • add gradle extension including unit tests
  • add maven extension
  • add FormatterStep to readme matrix
  • add example how to configure gradle-plugin readme
  • add gradle-plugin Changelog entry
  • add example how to configure maven-plugin
  • add maven-plugin Changelog entry
  • externalized defaults for gradle and maven into lib
  • test antlr4 + license headers with antlr grammars, lexers and parsers
  • test antlr4 + generic formatter steps
  • review by spotless team

plugin-gradle/README.md Outdated Show resolved Hide resolved
@matthiasbalke matthiasbalke changed the title Add ANTLR4 support [WIP] Add ANTLR4 support Dec 28, 2018
@matthiasbalke
Copy link
Contributor Author

matthiasbalke commented Dec 28, 2018

Just found something which seems to be a huge bug (antlr/Antlr4Formatter#25) in Antlr4Formatter. The bug seems to be removing parts of the grammar while formatting.

So please review the spotless integration but
do not merge the PR yet!

@nedtwigg
Copy link
Member

Looks great! Exemplary "add a language" PR. PaddedCell can be used to remedy idempotence problems, but it doesn't fix removing meaningful content :)

@matthiasbalke
Copy link
Contributor Author

Looks great! Exemplary "add a language" PR.

Thanks! 😊

# Conflicts:
#	CHANGES.md
#	plugin-gradle/CHANGES.md
#	plugin-gradle/README.md
#	plugin-maven/CHANGES.md
#	plugin-maven/README.md
@JLLeitschuh
Copy link
Member

This is awesome. I love how pluggable Spotless is. Does this have automatic maven support for it or does more work need to be done to add that in the future?

Another quick comment:
Looking at the antlr4formatter Github Project: https://github.com/antlr/Antlr4Formatter/graphs/contributors

It seems like it has very little activity and very little interactivity from the community. I just want to make sure that we aren't adding a framework to spotless that is at risk of not being maintained going forward (bit rot). Just something to consider @nedtwigg. It's not immediately a disqualifier for me, but I just want to let everyone think about it.

@matthiasbalke
Copy link
Contributor Author

This pr brings maven and gradle support. I know this project didn't have a lot of support. That's why I started to support it in December. Also I don't know about any other antlr4 formatter project. If you know one I be happy to take a look. Also the project is done by some of the official antlr4 guys, that is why I put my efforts there.

@nedtwigg
Copy link
Member

adding a framework to spotless that is at risk of not being maintained going forward

Everything is at risk of not being maintained :) I'm very much in favor of merging in support for marginal and esoteric formatters, so long as they don't require changes to our core, which this does not.

matthiasbalke and others added 3 commits June 5, 2019 22:31
# Conflicts:
#	CHANGES.md
#	plugin-gradle/CHANGES.md
#	plugin-gradle/README.md
#	plugin-maven/CHANGES.md
#	plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java
@nedtwigg
Copy link
Member

Looks like we're almost there! Let me know if you need anything from me.

@nedtwigg
Copy link
Member

I expect to publish a new release sometime this week. Do you think you'll get a chance to polish this up in that timeframe? If so I'm happy to wait to include this, else I'm just as happy to publish this whenever you get around to it :)

@nedtwigg
Copy link
Member

I'll lend a hand here.

@nedtwigg
Copy link
Member

If you force-push your branch to this location:
https://github.com/diffplug/spotless/tree/antlr4

then it will keep all the commits and discussion except for the recent rebase attempt. Thanks for your tenacity, I think it will be the oldest PR to actually get merged that I have ever worked with!

@matthiasbalke
Copy link
Contributor Author

matthiasbalke commented Jun 25, 2020

I don't quite understand, how force-pushing my work to the new branch, should remove the rebase attempt? Actually I merged the upstream/master into my local branch, so the branch name rebase is missleading ;) But if you want, I will polish the history later, when the build is back to green. Does that sound good?

The biggest problem I have right now, is that I'm having issues building spotless locally (Windows 10 and Ubuntu 20.04 using OpenJDK8). Do you have any idea, what I might do wrong?

$ ./gradlew test  

FAILURE: Build failed with an exception.

* Where:
Build file '/home/balke/repositories/spotless/ide/build.gradle' line: 11

* What went wrong:
A problem occurred evaluating project ':ide'.
> A problem occurred configuring project ':lib'.
   > java.lang.NullPointerException (no error message)

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 1s

And yeah I took me some time to get back to this, but I would love to get this merged too! Thanks for beeing so supportive.

@nedtwigg
Copy link
Member

Our primary branch is now main, not master. And because of that confusion, your PR was changing 237 files due to copyright header, which is not correct. I just did something very rude, and force-pushed over yours, but hopefully now you can see what I meant. I backed up your branch before force-pushing at matthias-backup, so if you really want to go back to that, you can. But I can't merge something that changed 100s of files, too hard to review.

@nedtwigg
Copy link
Member

I think now all of the test failures are about the expected input/output of the step, not related to infrastructure. I'll let you take it from here.

@nedtwigg
Copy link
Member

Now that this PR is nice and fixed-up, it's going to get merge-conflict clobbered again pretty soon, in ~week. I'm happy to work with you in realtime on https://gitter.im/diffplug/spotless if you have any hiccups. I think it's extremely close.

@matthiasbalke
Copy link
Contributor Author

@nedtwigg I just double checked the format with the ANLR4 grammar itself and the format is as expected now!

Thanks again for all the support!

This PR is finally ready to be merged.

@nedtwigg
Copy link
Member

nedtwigg commented Jul 1, 2020

Congrats on reaching a productive checkpoint on your long journey :D. I plan on releasing this (along with a bunch of other stuff) in a day or two.

@nedtwigg nedtwigg merged commit 5070536 into diffplug:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants