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

Toggle formatting inline with spotless:off and spotless:on #691

Merged
merged 20 commits into from
Sep 11, 2020

Conversation

nedtwigg
Copy link
Member

@nedtwigg nedtwigg commented Sep 10, 2020

Summary

Sometimes there is a chunk of code which you have carefully handcrafted, and you would like to exclude just this one little part from getting clobbered by the autoformat. Some formatters have a way to do this, some don't, but who cares. It would be nice if you could scatter spotless:off and spotless:on inline in your code, and have it just work, no matter which formatter you're using. This PR makes that possible.

Changing the defaults

The one hiccup is that, because it works for all languages / formatters, it isn't context-aware. Meaning that it doesn't care if spotless:off is in a comment, a string constant, or wherever. It just does a dumb search for pairs of spotless:off / spotless:on, and saves whatever is between them from being formatted. You can change the terms to be whatever you want, e.g. fmt:off, fmt:on, or whatever, but using something longer and uncommon like "spotless" is safer than something short like "fmt". You can also set a full regex if you want to try to make it comment-specific, but that's probably harder than it seems at first.

How it is implemented

It implements this under the hood with PipeStepPair, which has an In step and an Out step. The In step is the very first step which gets applied, Out is the last step, and all the other formatting steps go in between them. The In step finds matching off/on pairs, and stores the text between them. The intermediate steps like google-java-format or whatever do their work, and at the end the Out step finds the off/on pairs again, which may have moved. Then it replaces those pairs with whatever was stored by the In step.

Limitations

The intermediate steps can't remove an in/out pair. If they do, there's no way for spotless to put them back together. For example, if you have a step that converts your whole file to uppercase, that will turn spotless:off to SPOTLESS:OFF, which breaks the pair. This isn't a problem for the vast majority of steps. One notable exception, though, is the license header step. Because it replaces the whole header, any off/on pairs within the header will get clobbered, which will cause spotless to fail with an error.

@nedtwigg nedtwigg merged commit a443f5a into main Sep 11, 2020
@nedtwigg nedtwigg removed the request for review from jbduncan September 11, 2020 19:11
@nedtwigg nedtwigg deleted the feat/toggleOffOn branch September 11, 2020 19:11
@nedtwigg
Copy link
Member Author

Released in plugin-gradle 5.5.0 and plugin-maven 2.3.0.

Copy link
Member

@jbduncan jbduncan left a comment

Choose a reason for hiding this comment

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

Hi @nedtwigg, I finally found the time to review this.

I have quite a few comments, but other than that this is excellent work and I'm sure lots of google-java-format users and others will appreciate this. Well done! 👍

@nedtwigg
Copy link
Member Author

Thanks for the feedback @jbduncan! I'm going to open a draft PR that implements them, along with the beginning of implementing #412. I'm going to leave it as a draft until we have an actual project to test against.

@nedtwigg
Copy link
Member Author

nedtwigg commented Sep 14, 2020

Hi @jbduncan, I implemented some of the comments in the "Inception" PR linked above, and anything that I didn't implement I explained why. I'm happy to discuss further, or you're welcome to push code to this second "Inception PR" which cleans up the PipeStep a bit, and also adds some new functionality. The Inception PR is a lot more questionable than this one, so I'm going to wait until I've got feedback from you or some other stakeholder that it's the right approach before merging.

@jbduncan
Copy link
Member

@nedtwigg Just wanted to finally acknowledge your comments above, sorry it took so long! I'm happy with what's been done already, so I'm happy to bring this PR to a close close. :)

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.

2 participants