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

review: checkstyle: Spotless configuration #5984

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

micka-lama
Copy link

close #5741

I update the configuration of the Spotless plugin.
The existing configuration was too specific (the scan was limited to src/test/java/spoon/testing/assertions/**/*.java). The new one is global (only test classes are excluded, **/testclasses*/**).

The order of the imports is:

non-static
<blank>
static javax
static java
<blank>
static *

I also apply the IntelliJ basic style about the indentation (4 spaces) and whitespaces are trimmed.

Spotless documentation: https://github.com/diffplug/spotless/tree/main/plugin-maven

@I-Al-Istannen
Copy link
Collaborator

I haven't looked at it, but you need to preserve complete formatting for the generated assertions. It was limited to assertions deliberately: The generated tests do not follow a consistent formatting, so the spotless run is required to prevent changes on every execution. If you remove that, the generated code CI will fail on you. Just re-ordering import statements will not cut it for the generated code.

@micka-lama
Copy link
Author

Got it! Thank you

So the goal is to not have a git diff after applying Spotless on the generated classes? Is it ok for you if I commit with the new Spotless configuration only on the generated classes?

Initially, I didn't want to modify a lot of files here, just the pom.xml.

@I-Al-Istannen
Copy link
Collaborator

So the goal is to not have a git diff after applying Spotless on the generated classes?

Yes, exactly. The import order stuff (and also general formatting, I guess) would be nice for all of spoon. The generated assertions need to be completely managed by spotless though, so there is no diff after generation.

Is it ok for you if I commit with the new Spotless configuration only on the generated classes?

Sounds fine for a second commit in this PR, yea.

@micka-lama
Copy link
Author

micka-lama commented Oct 6, 2024

The import order stuff (and also general formatting, I guess) would be nice for all of spoon.

My mistake, you are right. I need to commit all the result of mvn spotless:apply (not only on src/test/java/spoon/testing/assertions/**/*.java). Otherwise, there will be a difference on the other part of the spoon project for the git diff.

@I-Al-Istannen
Copy link
Collaborator

If the reformat looks good and you are happy (and tests pass and the integrators are happy), you should create a .git-blame-ignore-revs file in the root of the repository with the commit hash of the reformat commit on its only line. I.e. if the hash of the reformat commit is c8481bc206ea28c0b7a5e88a64314e1ecb3179ce, you would create a .git-blame-ignore-revs file with the following content:

c8481bc206ea28c0b7a5e88a64314e1ecb3179ce

This will cause GitHub, git blame and most IDEs to ignore this commit when calculating blames. Makes the history a lot more usable, otherwise everything will be attributed to you.

@micka-lama
Copy link
Author

Because the strategy is a squash, the commit hash will be different compared to my local branch. It should be done in at least two PR, right? One, here, with the reformatting and another one with the .git-blame-ignore-revs file containing the hash of the squashed commit.

@algomaster99

This comment was marked as off-topic.

@I-Al-Istannen
Copy link
Collaborator

Are you happy with this PR, i.e. can I take a look at it? :)

@micka-lama
Copy link
Author

Are you happy with this PR, i.e. can I take a look at it? :)

Yes everything should be ok now! I am waiting for an eventual squash to create a second PR with the .git-blame-ignore-revs file.

@I-Al-Istannen
Copy link
Collaborator

I ported your changes onto the head of master to fix conflicts and separated configuration and formatting in separate commits (so it is easier to change in the future, if necessary).

Looking at the diff, I am also not sure if the formatting is a benefit or drawback. It also seems quite hard to get IJ to reproduce it.

WDYT @SirYwell @MartinWitt?

@micka-lama
Copy link
Author

Hello, any updates? It's been a while since the last activity. Please let me know if there's anything I can do.

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.

Spotless for imports
3 participants