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

Enforce formatting with spotless #23

Merged
merged 6 commits into from
May 11, 2022
Merged

Enforce formatting with spotless #23

merged 6 commits into from
May 11, 2022

Conversation

tsaglam
Copy link
Member

@tsaglam tsaglam commented Apr 14, 2022

This is an initial PR to support formatting/code style with spotless thus addressing vitruv-tools/Vitruv#495 and vitruv-tools/Vitruv#497. It allows to check these guidelines via mvn spotless:check and apply them automatically with mvn spotless:apply.

It includes:

  • An initial Eclipse-based formatting configuration
  • A spotless configuration as part of the parent pom

Decisions to make:

  • Do we want pom re-ordering or not? Yes!
  • Do we want tabs or spaces (in Java/Xtend/Reactions/pom files) Tabs!
  • Which formatting guidelines do we want for Java (the current formatter.xml is taken from JPlag)?
    • e.g. line width/comment line width
  • Do we want to include the spotless check in the mvn build phase (running the check every time when building vitruv, even locally) or just enforce it via a Github action workflow?

Left to do:

  • Apply spotless in a big-bang manner to the repos
  • Add Github Action workflows for spotless:check to the repos

EDIT: Note that this PR does not yet include workflows to run spotless checks on incoming PRs. Thus, we can merge this PR and then add the missing workflows whenever we want to start enforcing these style guidelines.

@tsaglam tsaglam added the enhancement New feature or request label Apr 14, 2022
@HeikoKlare
Copy link
Contributor

Thanks for proposing the formatting enforcement! My responses on the decisions to make:

Re-ordering of poms : Sounds reasonable to me to unify POMs as well. Are there any drawbacks I don't see?

Tabs/spaces: As you know, I am in favor of tabs, but spaces are also fine for me. I think that currently most of the files are formatted with tabs, but that might not be an argument in favor of one or the other

Formatting guidelines: The ones from JPlag are fine for me. We should just change the name of the formatter as it still contains "JPlag" 😉

Check during every build: Couldn't we put the spotless plugin into a Maven profile, which you can activate locally on demand with something like -Pspotless and always use that profile in the GitHub Actions?

@tsaglam
Copy link
Member Author

tsaglam commented Apr 29, 2022

Re-ordering of poms : Sounds reasonable to me to unify POMs as well. Are there any drawbacks I don't see?

Only that the initial spotless formatting commit of each repo might change each pom file a lot. After that, they all will be consistently ordered though.

Formatting guidelines: The ones from JPlag are fine for me. We should just change the name of the formatter as it still contains "JPlag" 😉

Will do! 😄

Check during every build: Couldn't we put the spotless plugin into a Maven profile, which you can activate locally on demand with something like -Pspotless and always use that profile in the GitHub Actions?

Actually, by default one can run mvn spotless:check to check the formatting mvn spotless:apply to automatically format everything. Maybe a bit of background for this discussion point: With JPlag we see quite a few PRs with git logs like this:

* commit 1: implement a feature
* commit 2: fix spotless issues

This is the case because people that are less familiar with the project might not run spotless before committing or pushing.
When opening a PR the CI check will tell them that spotless failed, leading to them fixing the issues in another commit.
Cases like this can be avoided if the default behavior of the mvn build includes spotless checking.
However, sometimes you only want to run the build and see if it compiles/tests pass and want to fix style later, then this might be an inconvenience. This might be an argument for keeping the build (by default) without the spotless check). In the JPlag build pipeline, we currently do not include a spotless check by default. For vitruv, I am open for doing it either way.

@JanWittler
Copy link
Contributor

Pom reordering: 👍🏼

tabs / spaces: I am used to spaces but as the IDE should do it automatically when integrating the provided formatter (if I understand correctly?) I don't really care. I couldn't find any strong arguments with a short research for any side.

Formatting guidelines: Fine with provided ones

Maven build: I would separate functionality and formatting so don't check formatting in the default build. Having some spotless issue fix commits won't hurt anyone (and contributors should learn this after once mistakenly commit wrong formatted) but running the format checks by default in every local build is an unnecessary overhead.

@HeikoKlare
Copy link
Contributor

Pom reordering: 👍

Maven build: I am fine with both. I would also be fine with spotless checks by default (which we might even allow to disable with a specific profile). What happens if spotless checks fail? Does the complete build fail or does it only provide warnings (or can we configure that)? Having checks with warnings in the default build would be the best option in my opinion.

@tsaglam
Copy link
Member Author

tsaglam commented Apr 29, 2022

What happens if spotless checks fail? Does the complete build fail or does it only provide warnings (or can we configure that)? Having checks with warnings in the default build would be the best option in my opinion.

By default it fails the build. An example can be seen here.
I will look into the Spotless documentation to see if it can be configured to provide warnings.

@SofiaAnanieva
Copy link

Thank you for the effort!

POM re-ordering: 👍

Tabs/spaces: No strong opinion here. Personally, I would prefer tabs.

Formatting guidelines: The ones from JPlag are also fine with me 👍

Maven build: I would include spotless in the default maven build, but only issue warnings (i.e., no build failure if spotless checks fail). In the GitHub action workflow the build should fail if any spotless checks fail. Otherwise, warnings will just be ignored. Like Jan, I don't see much harm in the occasional spotless issue fix commits.

@tsaglam
Copy link
Member Author

tsaglam commented May 10, 2022

To my knowledge, it is not possible to configure spotless to only show the issues as warnings. This means we either check with spotless during every mvn build and the build fails if there are style issues. Or we only check the style for PRs via a workflow and we ourselves need to call spotless explicitly during development if we want to know if there are issues.

@tsaglam tsaglam merged commit 1996166 into master May 11, 2022
@JanWittler JanWittler deleted the spotless branch May 12, 2022 07:01
JanWittler pushed a commit that referenced this pull request Aug 17, 2022
This reverts commit 1996166, reversing
changes made to b518de2.
JanWittler pushed a commit that referenced this pull request Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants