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.

Sign up for GitHub

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

Jump to bottom

[BUILD] Check java formatting in Travis CI #313

Merged
merged 3 commits into from
Nov 26, 2018
Merged

[BUILD] Check java formatting in Travis CI #313

merged 3 commits into from
Nov 26, 2018

Conversation

m273d15
Copy link
Contributor

@m273d15 m273d15 commented Nov 23, 2018

  • Introduced Formatting stage in travis
  • Created script for downloading and executing
    the google java formatter

@m273d15 m273d15 force-pushed the pr/build/travis_format_check branch from 682f86d to dd8a976 Compare November 23, 2018 09:39
@m273d15
Copy link
Contributor Author

m273d15 commented Nov 23, 2018 •
edited
Loading

See #303 for further information

The failing check is already visualized in GitHub checks

stefaus
stefaus reviewed Nov 23, 2018
View reviewed changes
travis/script/format/check_java.sh Outdated Show resolved Hide resolved
travis/script/format/check_java.sh
local jar_name=$1

echo 'Checking format'
java -jar "$jar_name" --dry-run --set-exit-if-changed **/*.java
Copy link
Contributor

@stefaus stefaus Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If just check changed files is enough you could maybe use the file list of git diff master --name-only --diff-filter=d.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is necessary to optimize this for now. A basic setup is enough for the beginning. If it slows down the builds to much, we can improve it.
(When creating the git diffs, you would have to you the correct base branch to compare against as not all builds are against the master. An easy example are builds for stacked PRs. Kelvin mentioned that this would be possible through travis variables, but I am not certain we really need it for now.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

I don't think it's a problem to always diff against the master, because yes in a Stacked PR you would check all PRs/commits in the chain but that is not a real problem isn't it?

tobous
tobous reviewed Nov 23, 2018
View reviewed changes
Copy link
Member

@tobous tobous left a comment

Choose a reason for hiding this comment

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

Looks good to me (except for using "Formatter.jar" directly, which Stefan already mentioned).

I would suggest to also include the formatting in this PR as a separate commit. But this can be done shortly before merging it (after the gradle stuff).

@m273d15 m273d15 force-pushed the pr/build/travis_format_check branch from dd8a976 to ced4a5c Compare November 26, 2018 07:37
@m273d15
[BUILD] Check java formatting in Travis CI
57f7bfb 
* Introduced Formatting stage in travis
* Created script for downloading and executing
  the google java formatter
@m273d15 m273d15 force-pushed the pr/build/travis_format_check branch from ced4a5c to 57f7bfb Compare November 26, 2018 10:01
@m273d15
Copy link
Contributor Author

m273d15 commented Nov 26, 2018

@tobous Added corresponding formatting commit.

@tobous
Copy link
Member

tobous commented Nov 26, 2018

How about also removing the old fotmatting Config files as they are no longer useful and could give users the impression that they are using the correct formatting?

@m273d15
[NOP] Remove old formatting files
001bca4 
* Remove old IntelliJ formatting configs
* Remove old eclipse formatting profile
@m273d15
[NOP] Reformat code with Google Java Formatter
b965321
@m273d15 m273d15 force-pushed the pr/build/travis_format_check branch from c7515a1 to b965321 Compare November 26, 2018 10:46
@m273d15
Copy link
Contributor Author

m273d15 commented Nov 26, 2018

I removed IntelliJ code style settings and the eclipse formatting profile. I did not remove the eclipse clean-up profile, because it configures things like (remove unused imports etc.) and not formatting.

stefaus
stefaus approved these changes Nov 26, 2018
View reviewed changes
Copy link
Contributor

@stefaus stefaus left a comment

Choose a reason for hiding this comment

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

.settings/org.eclipse.jdt.ui.prefs Files still mention a Saros Formater, but the rest looks good to me

@m273d15 m273d15 merged commit fb99809 into master Nov 26, 2018
@fzieris
Copy link
Member

fzieris commented Nov 26, 2018

The files in the "patches" folder were always excluded from any auto-format commits. Those are "original" pieces of other projects' code with minor amendments. Auto-formatting these makes it way harder to see the actual "patch".

@m273d15 m273d15 mentioned this pull request Nov 26, 2018
Closed
@m273d15 m273d15 deleted the pr/build/travis_format_check branch November 27, 2018 14:09
@m273d15 m273d15 mentioned this pull request Nov 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefaus stefaus stefaus approved these changes

@tobous tobous tobous approved these changes

@Drakulix Drakulix Awaiting requested review from Drakulix

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m273d15 @tobous @fzieris @stefaus