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

Partial GJF format #99

Merged
merged 1 commit into from
Aug 4, 2017
Merged

Conversation

jbduncan
Copy link
Contributor

@jbduncan jbduncan commented Aug 4, 2017

Issue: #86.

This is a second attempt (following #87) to format the JUNG code base with google-java-format. Note however that it's a work-in-progress, for reasons explained in the commit message, which I've copied and pasted below.

The Maven plugin https://github.com/coveo/fmt-maven-plugin was used
to do this job. Those files which are still in the process of being
removed or otherwise dealt with for JUNG 3.0 (as described in
https://github.com/jrtom/jung/pull/87) have been left alone to
prevent unnecessary work and merging problems.

The formatting has not yet been tweaked in places where
google-java-format itself cannot automatically do so but which would
be beneficial for readability, like:
1. Removing superfluous empty lines at the start and end of method
   bodies.
2. Adding braces to if, for and while statements.

The formatting has also not been checked to see if javadocs have been
adversely affected.

The Maven plugin https://github.com/coveo/fmt-maven-plugin was used
to do this job. Those files which are still in the process of being
removed or otherwise dealt with for JUNG 3.0 (as described in
jrtom#87) have been left alone to
prevent unnecessary work and merging problems.

The formatting has not yet been tweaked in places where
google-java-format itself cannot automatically do so but which would
be beneficial for readability, like:
1. Removing superfluous empty lines at the start and end of method
   bodies.
2. Adding braces to if, for and while statements.

The formatting has also not been checked to see if javadocs have been
adversely affected.
@jrtom
Copy link
Owner

jrtom commented Aug 4, 2017

There are some things about how google-java-format does things that I don't 100% agree with, but this is a vast improvement over the current state of things. Thanks! :)

@jrtom jrtom merged commit 8f2173e into jrtom:common.graph Aug 4, 2017
@jbduncan
Copy link
Contributor Author

jbduncan commented Aug 4, 2017

There are some things about how google-java-format does things that I don't 100% agree with, but this is a vast improvement over the current state of things. Thanks! :)

Fair enough. I don't think there are many people out there who agree with everything google-java-format does (I know there is at least one thing I don't necessarily agree with), but overall I like it quite a bit, to the point where I think it's better than Eclipse's formatter.

You're very welcome. :)

I still have to manually check each and every if/for/while statement to turn them into block forms, which would arguably look better when formatted with google-java-format, so I'll submit another PR in the future.

I also have to make google-java-format formatting & checking part of the Maven build, but that will come when all of the old graph-related classes are gone.

Personally, I'm not too concerned about the possibility that google-java-format may have messed up the formatting of the javadoc. It looked to be in a bit of a sorry state before, so it was probably going to need work at some point anyway. :)

@jbduncan jbduncan deleted the partial-gjf-format branch August 4, 2017 19:16
@jrtom
Copy link
Owner

jrtom commented Aug 4, 2017

Understood and agreed that there's a lot of blockifying to do. I think that's something that the Eclipse formatter can do, so it might be easiest to just run it over everything (if there's a batch mode for doing that, I forget).

Also agreed that the Javadoc formatting was at least not in a very consistent state previously. (I did a cleanup pass for the 2.1 release, so it used to be worse, but it still wasn't pretty.)

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