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

Reformat in line with the Google Java Style Guide #182

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

Conversation

tomwhite
Copy link
Member

@tomwhite tomwhite commented Feb 5, 2018

Alternative approach to #153.

@coveralls
Copy link

coveralls commented Feb 5, 2018

Coverage Status

Coverage decreased (-0.02%) to 63.948% when pulling e59cdec on tomwhite:style into 31bd3a9 on HadoopGenomics:master.

@heuermh
Copy link
Contributor

heuermh commented Feb 5, 2018

If I have it right, in this PR you've only reformatted for whitespace?

What would your plan be for braces and other considerations? The Google Java Style Guide makes recommendations on package names

package org.seqdoop.hadoop_bam;

and camel case in class names

public class AnySAMInputFormat

that would require API compatibility breaking changes to implement.

@tomwhite
Copy link
Member Author

tomwhite commented Feb 6, 2018

@heuermh This patch is for more than whitespace - it includes brace placement too, for example - but it doesn't change package or class names. I'm not sure about doing that latter because it's a breaking API change.

The present change implements most of the Google Java Style Guide (those implemented by https://github.com/google/google-java-format) and makes it easy for us to ensure future patches comply.

@fnothaft
Copy link
Contributor

fnothaft commented Feb 6, 2018

I'm not sure about doing that latter because it's a breaking API change.

We're planning that the next rev is a major version bump (7.9.1 -> 8.0.0), right? If so, this is a great opportunity to make breaking API changes. ;)

@tomwhite
Copy link
Member Author

tomwhite commented Feb 6, 2018

That's right. However, just because we can, doesn't mean we should. Anyway, it doesn't need doing in this PR.

@heuermh
Copy link
Contributor

heuermh commented Feb 6, 2018

This patch is for more than whitespace - it includes brace placement too, for example...

It missed some then, e.g. AnySAMInputFormat.java, line 122

if (fmt != null || formatMap.containsKey(path)) return fmt;

if (givenMap)
  throw new IllegalArgumentException("SAM format for '" + path + "' not in given map");

if (this.conf == null) throw new IllegalStateException("Don't have a Configuration yet");

https://google.github.io/styleguide/javaguide.html#s4.1.1-braces-always-used

Some things will probably have to be done by hand; I started down this path in #154. How about we add additional non-breaking API style commits to this PR, and then consider breaking API changes separately?

@fnothaft
Copy link
Contributor

fnothaft commented Feb 6, 2018

That's right. However, just because we can, doesn't mean we should. Anyway, it doesn't need doing in this PR.

Hi @tomwhite! I'd appreciate hearing a bit more of your perspective here. From my perspective, these are cosmetic API changes (rather than functional API changes), and it would be preferable to be fully in-line with our chosen style guide instead of having inconsistency between new and old code. #181 introduces breaking changes, so if we're going to have to do a major version rev, we might as well fully pay off a stylistic smell while we're at it.

@tomwhite
Copy link
Member Author

tomwhite commented Feb 7, 2018

@heuermh Good catch. It seems that when I use the Google Java Style Guide IntelliJ plugin (https://github.com/google/google-java-format) or the Maven Plugin, they miss some changes. When I get IntelliJ to reformat using https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml, then it catches the ones you pointed out.

So the best thing is to get both IntelliJ to reformat, and the Maven Plugin to format after that (since it typically has some extra changes due to whitespace).

I have also done #168 as a part of this.

@fnothaft let's consider API breaking changes in a separate issue.

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.

4 participants