-
Notifications
You must be signed in to change notification settings - Fork 19
style: add Spotless formatter configuration #189
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
Conversation
|
Since this PR formats code, it will cause a lot of conflicts with other PRs. |
| public PackageURL( | ||
| final String type, | ||
| final String namespace, | ||
| final String name, | ||
| final String version, | ||
| final TreeMap<String, String> qualifiers, | ||
| final String subpath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change has something to do with the number of parameters? Maybe increase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatter limits the line to 120 characters and starts wrapping if it's more.
| */ | ||
| public MalformedPackageURLException() { | ||
| } | ||
| public MalformedPackageURLException() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am against this change since braces are not supposed to be on a single line, empty or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a choice in the palantir-java-format, not modifiable.
| throw new MalformedPackageURLException( | ||
| "The PackageURL " + component + " '" + value + "' contains invalid characters: " + invalidChars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe increase the line length? What is it currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line length chosen by the palantir-java-format is 120. Non-modifiable.
In general I like the fact that the formatter does not accept any parameters. At a previous job we spent 3 months deciding on all the hundreds of options of the Eclipse formatter and everybody hated the result anyway.
| private static void validateChars(String value, IntPredicate predicate, String component) | ||
| throws MalformedPackageURLException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is a line length issue as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in fc1972e has been generated by running:
mvn spotless:apply
|
@ppkarwasz due to how GH works, I can't figure out how to easily merge this without fixing the merge conflicts. While that sounds easy (revert the last commit, rebase or merge master, run spotlessApply), doing this on someone else's PR appears to be a bit more challenging. Mind updating this one? |
f155571 to
e30d6f3
Compare
e30d6f3 to
52b6dfc
Compare
52b6dfc to
1e444e4
Compare
|
@jeremylong, done! |
|
Thanks! |
This PR adds a Spotless configuration that uses:
palantir-java-formatformatter for Java files.sort-pomformatter for the POM file.Review tips
This PR contains two commits:
Closes #183