triplea-game / triplea

TripleA is a turn based strategy game and board game engine, similar to Axis & Allies or Risk.
https://triplea-game.org/
GNU General Public License v3.0
1.32k stars 392 forks source link

Code auto-format #4488

Closed DanVanAtta closed 5 years ago

DanVanAtta commented 5 years ago

I'd like to propose we adopt strong code formatting automation to remove that burden from development and review.

Way back in #18 we agreed to follow Guava java formatting. Fast forward to now and we can do a better job of sticking to that spec.

As an agreement we made an optional allowance for long line length wrapping, but at some point the formatter wraps various elements, like comments, which does not make it optional nor follows guava java format. This additional means we maintain a config file to specify the formatting.

After getting a bit used to not doing any hand-formatting, I'll say it's a surprisingly nice productivity booster. To note some explicit benefits:

Re-reading #18, the guava plugin is essentially this:

On the actual topic of formatting, I would personally like to be able to auto-format in both eclipse and intellij, without any difference at all between them. That would be great, and it should be easy to implement without having to download or install a lot of crap, or spend hours configuring things.

The plugins are available at: https://github.com/google/google-java-format, no configuration other than turning the formatting on, installs quickly

ssoloff commented 5 years ago

we agreed to follow Guava java formatting

Just to clarify, you mean Google Java Style, not Guava Java Style, correct?

After getting a bit used to not doing any hand-formatting, I'll say it's a surprisingly nice productivity booster.

Agreed. That's why I have my IDE configured to run the formatter after every save. :smile:

guava formatter build integrations can let us get even more mileage making, removing these entirely let's us focus

If for some reason we decide to stick with the current formatter, it can also be integrated into the build using Spotless (which supports both Google and Eclipse formatters).


On its face, I don't have any real objections to switching formatters. However, I'd like to see exactly how the Google formatter handles our codebase. So, a next step would be to run it on everything and see how things would change.

One thing I'm concerned about is that Google Java Style is rather permissive when it comes to certain formatting (i.e. the style guide has no opinion--by design--on certain things). For example, in the recent PRs that generated this discussion, it looks like the Google formatter permits field annotations either on the same line or on a separate line, whereas the Eclipse formatter is configurable to force it either way. As you said above, the primary purpose of an auto-formatter is to take out all decisions about formatting so that it looks like everything was written by one person. If half the team likes same-line annotations and the other half likes separate-line annotations, and the formatter doesn't care, the code style is going to diverge.

My preference is that whichever formatter we use, when applied to any dev's code, generates as close to 100% identical results as possible. This should include Javadocs, as well.

DanVanAtta commented 5 years ago

Just to clarify, you mean Google Java Style, not Guava Java Style, correct?

I did not realize there was a difference :blush:
To answer the question directly, from !18, this: https://google.github.io/styleguide/javaguide.html

However, I'd like to see exactly how the Google formatter handles our codebase. So, a next step would be to run it on everything and see how things would change.

A mass formatting example can be viewed at: https://github.com/triplea-game/triplea/pull/4495

For consideration:

One thing I'm concerned about is that Google Java Style is rather permissive when it comes to certain formatting

AFAIK, the only case of permissiveness is blank lines, and maybe some leeway in html markup. I think you'll find the formatter is actually quite opinionated. For the case of same-line vs next-line for the field annotation, the formatter is looking at the resulting line length and does same-line when it fits.

I think the two questions to answer here are:

  1. are we agreed to roll-out the google-java-format plugin?
  2. if agreed, how do we roll-it-out?
    • for consideration:
      • google-java-format can be run automatically by gradle, travis could kick that off and check-in any changes to PR branches using the builder bot account. To roll-out the formatting updates, we'd only need to submit the travis changes and the PR branch would update automatically to reformat the whole project.
      • in case 'allow updates by maintainers' is not selected and travis cannot update the PR branch, we can choose to set travis/gradle to fail the build if there are any unformatted files
      • an alternative, we could have travis always fail the build if there any unformatted files and merge a one-time mass format, leaving it up completely up to devs to run the formatter.

My preference is that whichever formatter we use, when applied to any dev's code, generates as close to 100% identical results as possible. This should include Javadocs, as well.

In this case, I think you will like the formatter plugin. AFAIK a notable difference between new and old is that our existing formatter will only line-break javadocs, the proposed new will do that and more going as far as adding/removing tags.

DanVanAtta commented 5 years ago

Given the failures from the demo branch: https://travis-ci.org/triplea-game/triplea/jobs/475041398, (checkstyle), we'll need to reconcile our checkstyle rules as well.

DanVanAtta commented 5 years ago

Here is a good concrete example of code I am working on and turned the formatter on to get formatting help. First is an example with our current formatter, second is the same block with google-java-format:

package games.strategy.debug.error.reporting;

import java.util.function.BiConsumer;

import javax.swing.JFrame;

import org.triplea.http.client.error.report.ErrorReportClientFactory;

import games.strategy.engine.lobby.client.login.LobbyPropertyFetcherConfiguration;

public class ErrorReportConfiguration {
  public static BiConsumer<JFrame, UserErrorReport> newReportHandler() {
    return LobbyPropertyFetcherConfiguration.lobbyServerPropertiesFetcher().fetchLobbyServerProperties()
        .<BiConsumer<JFrame, UserErrorReport>>map(lobbyServerProperties -> new ErrorReportStrategy(
            ErrorReportClientFactory.newErrorUploader(lobbyServerProperties.getHttpServerUri())))
        .orElse(ErrorReportStrategy.OFFLINE_STRATEGY);
  }
}

After:

package games.strategy.debug.error.reporting;

import java.util.function.BiConsumer;

import javax.swing.JFrame;

import org.triplea.http.client.error.report.ErrorReportClientFactory;

import games.strategy.engine.lobby.client.login.LobbyPropertyFetcherConfiguration;

public class ErrorReportConfiguration {
  public static BiConsumer<JFrame, UserErrorReport> newReportHandler() {
    return LobbyPropertyFetcherConfiguration.lobbyServerPropertiesFetcher()
        .fetchLobbyServerProperties()
        .<BiConsumer<JFrame, UserErrorReport>>map(
            lobbyServerProperties ->
                new ErrorReportStrategy(
                    ErrorReportClientFactory.newErrorUploader(
                        lobbyServerProperties.getHttpServerUri())))
        .orElse(ErrorReportStrategy.OFFLINE_STRATEGY);
  }
}
DanVanAtta commented 5 years ago

@RoiEXLab @ron-murhammer any objections/thoughts on google-java-format? It's a more complete formatter and handles nearly all formatting decisions. To use it, it's an IDE plugin that would need to be installed.

ron-murhammer commented 5 years ago

@DanVanAtta I'm open to it. I'm not 100% clear what you are proposing. Is it you want to use a different formatter plugin instead of built in IDE? Or that you want to change the formatting rules we are using? Or both? I don't particularly have much issue with what we currently have and I almost never hand format any code as I use format on save.

RoiEXLab commented 5 years ago

I don't have a problem adopting a new plugin to do that if only everything is consistent. I learned to love the ESLint auto-fix functionality, so that's nothing new for me. I don't like all the design choices in the "proposed" PR ~but as long as the plugin is configurable~, but I might get used to it, as always

RoiEXLab commented 5 years ago

Just realized: If the plugin isn't configurable, this will mean we're limited to 100 characters per line instead of 120, right?

For me that would be a huge deal breaker to use this exact plugin.

ssoloff commented 5 years ago

I looked through the preview PR and am okay with like 75% of the formatting changes--the rest I can get used to. :smile:

However, I do agree with @RoiEXLab that falling back to a 100-column line length is going to be somewhat of a pain. Not simply because I think 120 columns is perfect for screens 1920x1080 and above, but because I think we'll find some of the "newly-condensed" code a bit harder to read because of all the extra indentation.

Regardless of whether we switch formatters, I'm still all for integrating something like Spotless into the build to force us all to use the same formatting so that it no longer has to be discussed during review.

DanVanAtta commented 5 years ago

The non-configurable by design is intended, so we cut out on any and all preference debates.

Please note the example I posted: https://github.com/triplea-game/triplea/issues/4488#issuecomment-451354530

That is an actual example, the top-version is our current formatter, and it looks bad. After hand-formatting, it would look okay, and that is something I know I encounter surprisingly frequently.

Regardless of whether we switch formatters, I'm still all for integrating something like Spotless into the build to force us all to use the same formatting so that it no longer has to be discussed during review.

Not simply because I think 120 columns is perfect for screens 1920x1080 and

For consideration, as mentioned in !18, google researching this and screen-size in IDE is not the only place to optimize for. Not having to scroll left or right in PR reviews I personally find very nice. Regardless, part of the point is that this formatter is google-java standard, and it makes all formatting decisions for us. I really don't want us to have the line length discussion (again), or any other similar, hence the why the plugin is "not-configurable by design"

DanVanAtta commented 5 years ago

@ron-murhammer to answer directly:

I'm open to it. I'm not 100% clear what you are proposing.

I'm proposing we start using google-java-format plugin to do all formatting. It is a "strong" version of google java format, which is essentially something we agreed to some years ago and have made best-efforts to do rather than having the actual formatter.

Is it you want to use a different formatter plugin instead of built in IDE?

Technically we are using a custom configuration and pointing our IDE to it. Instead of that, there is a plug in to install and then the IDE format will use the plugin-in. Any config files we have for formatting could be deleted.

Or that you want to change the formatting rules we are using?

The rules would not change per-say, but the proposed plug-in would follow those rules to a 'T'. The current eclipse configuration we have is a best-effort towards google java format.

I don't particularly have much issue with what we currently have and I almost never hand format any code as I use format on save.

Indeed, but the current format on save, as noted: https://github.com/triplea-game/triplea/issues/4488#issuecomment-451354530, can fall short (and often so). Furthermore, we are still left with various design decisions when it comes to formatting.

The dedicated plug-in for formatting is 'smart' in some ways and will line-break only when you have to, it makes more choices than just the IDE formatter with custom configuration rules.

ron-murhammer commented 5 years ago

@DanVanAtta I guess I'd vote no then as I agree with @RoiEXLab 100 column line length is too painful. That is really the only thing I strongly disagree with with the standard google java format.

ssoloff commented 5 years ago

One other thing I'm observing is that there appear to be lines in Google-formatter-formatted code with trailing whitespace. For example, line 69 below:

https://github.com/triplea-game/triplea/blob/f208dac52cc9114deb144ebe6a2852ad7fd08d03/game-core/src/main/java/games/strategy/engine/lobby/client/login/LobbyServerPropertiesFetcher.java#L68-L70

It may be that that whitespace sneaked in before @DanVanAtta enabled auto-format-upon-save. It would be surprising if the Google formatter does not trim trailing whitespace.

DanVanAtta commented 5 years ago

@ssoloff It's neither formatter, I do not have format-upon-save enabled. The example code above is frequent enough that I save a lot of time by not hand-formatting it. Frankly something like that compared to line length (very subjective) I think is a bit myopic.

The net result is that I try to be careful about which code I format, and files I author or significantly update then I'll format-all.

DanVanAtta commented 5 years ago

@DanVanAtta I guess I'd vote no then as I agree with @RoiEXLab 100 column line length is too painful.

Are there good examples demonstrating this pain? I know that every stream, builder, loop, javadoc requires hand-formatting right now; that is quite painful, see this example: https://github.com/triplea-game/triplea/issues/4488#issuecomment-451354530

On the other hand, not caring about how the code is formatted at all is really nice. Our current formatter is not that strong where you could remove all line breaking and then reformat and have something consistently clean. I should know, as the first mass reformat I somewhat in protest hand-formatted many things that did not look so clean. Had we used java-google-format, such a hand-format would not have been needed and would have been undone anyways by formatting (it is a stronger formatter, strong-enough where we can practically stop caring about format 100%)

DanVanAtta commented 5 years ago

Another aspect to consider, since our formatter leaves many decisions open, we have the potential for formatting nits on PRs.

Recent example where I was tempted to format nit the double and on same line: https://github.com/triplea-game/triplea/pull/4498/files#diff-be49d31cd69628c997c237fb05308746R1525

I let this slide as it's a formatting nit and figured we'd get stuff like this updated. If our formatter were 100% in deciding our format, there would be no debate, it'd just be the formatter output and we could stop discussing formatting from here on.

DanVanAtta commented 5 years ago

@ssoloff

I do not have format-upon-save enabled.

This is for two reasons:

  1. It's a bit dangerous
  2. The performance of the tool is not perfect, there is a noticeable performance hit from the save-actions plugin and it trips up over eclipse save formatter plugin
DanVanAtta commented 5 years ago

I think we'll find some of the "newly-condensed" code a bit harder to read because of all the extra indentation.

Hmm, most of the indentation is actually from optional breaking rather than end of line limits. Having run-on code be formatter to 100 character columns is not easy to read and needs hand formatting. I think this would only be the case thanks to those manual efforts. Since it is optional line breaks that dominate indentation, I think it's an incorrect perception that line length dominates indentation.

Again though, I think this is totally subjective, distracting, and kinda pointless thing to even debate. Having format decisions is not a good thing, it's one more damn thing to worry about. Our current formatter leaves many of those decisions up to us, the fact we can even have this debate I think is misguided. I'm suggesting we shut this down, stick to the 'use this 3rd party, non-debatable standard', and remove these decisions once and for all.

DanVanAtta commented 5 years ago

Sorry to rant here, but I think there is a lack of knowledge and we're not on a common ground yet.

Our current formatter has hard and fast rules on which elements are same vs next line, and otherwise breaks based on line length. google-java-format is 'smart' and breaks based on element types. For example if parameters span to multiple lines then it will format them one per line. Our current formatter would just line break at 100. eg:

    final List<Predicate<Territory>> prioritizedMovePreferences = new ArrayList<>(Arrays.asList(
        hasRequiredUnitsToMove.and(notEnemyOwned).and(noEnemyUnits),
        hasRequiredUnitsToMove.and(noEnemyUnits), hasRequiredUnitsToMove.and(noAa),
        notEnemyOwned.and(noEnemyUnits),
        noEnemyUnits,
        noAa));

^ the above is allowed by current formatter, an example where it just breaks based on long lines only (did you notice one line has multiple args?). Removing all line breaks and formatting, there is a difference from what we have, that shows there is non-determinism and hand-formatting involved:

    final List<Predicate<Territory>> prioritizedMovePreferences =
        new ArrayList<>(Arrays.asList(hasRequiredUnitsToMove.and(notEnemyOwned).and(noEnemyUnits),
            hasRequiredUnitsToMove.and(noEnemyUnits), hasRequiredUnitsToMove.and(noAa), notEnemyOwned.and(noEnemyUnits),
            noEnemyUnits, noAa));

The above I do not find necessarily that easy to read, it's not bad, but params per line makes it really clear with the and lining up.

My point here is that line length is a factor for a naive formatter that is mostly breaking only based on line length. That 80 vs 100 line length is coming up at all I think is missing this important point.

DanVanAtta commented 5 years ago

TL;DR 80 vs 100 is myopic, only a factor because current formatter is not very strong. EG: if we removed all line breaks and reformatted, would we be happy with the result? The answer I believe is no, example above. Given that our code formatting is actually nice, we've gotten there thanks to hand-formatting. By saying we want our current formatter, it is saying we want to continue hand formatting code instead of worrying about higher-order problems, and that we're okay having such subjectivity which allows it to even come up for debate (wasting time and focus in PRs).

I haven't had to worry about formatting for some time, I hope to bring the same to this project so we can end this debate and have all code just be 100% formatted for us, let's worry about more important things (which means stop hand-formatting).

DanVanAtta commented 5 years ago

@ron-murhammer Let me revise my proposal: That we stop hand-formatting. In essence, remove all line-breaks, and reformat. With the current formatter that will not turn out so nicely. Do you have alternative auto-formats to consider? Are we doomed to hand-formatting code? Do we really want to be reviewing that code is formatted?

tvleavitt commented 5 years ago

I've been watching this discussion. It seems that the major objection to using the Google Java Formatter is that the maximum line length is set to 100 instead of 120. Given that, since the source code for that formatter is presumably open source, why not simply modify it to max out at 120 lines (if there's no way to configure it), and be done? You get all the other benefits outlined, and can quit spending energy on this discussion and formatting at all. Line length is an arbitrary number anyway, so why not just pick the arbitrary number that folks with a concern find acceptable and go from there?

Regards, Thomas

On Sat, Jan 5, 2019 at 1:45 PM Dan Van Atta notifications@github.com wrote:

@ron-murhammer https://github.com/ron-murhammer Let me revise my proposal: That we stop hand-formatting. In essence, remove all line-breaks, and reformat. With the current formatter that will not turn out so nicely. Do you have alternative auto-formats to consider? Are we doomed to hand-formatting code? Do we really want to be reviewing that code is formatted?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/triplea-game/triplea/issues/4488#issuecomment-451693146, or mute the thread https://github.com/notifications/unsubscribe-auth/AEPTgYcvkBd8u8yIja3IWie1jA65oEIOks5vAR0HgaJpZM4Znr5_ .

-- Thomas Leavitt Internet enabled since 1990

ssoloff commented 5 years ago

Another option is to just tighten up the Eclipse formatter configuration so that its line wrapping policy conforms to the Google formatter's. The Eclipse formatter has over 300 configuration parameters--about half of those related to line wrapping. I suspect it could be configured to match--to a very close degree--the Google formatter's line wrapping style. At the same time, the line length can remain at 120 columns. That should resolve the various forces enough to make the format acceptable to everyone.

DanVanAtta commented 5 years ago

At the end of the day IDC so much as long as we can stop spending time doing any hand-formatting.

The examples where our current formatter does nothing and leaves hard to read code I think are very compelling.

@ssoloff, I'd rather us not maintain the configuration rules, we could drop it all in favor of an out-of-the-box plugin. Ignoring that, the current has a performance hit with save-actions and eclipse-formatter and sometimes can have problems.

I'm curious for examples where 100 vs 120 really matters. @ron-murhammer, @RoiEXLab, in which cases is line length very important (in comments, params, general code)?

@tvleavitt, I like the suggestion, though would be good to do a fork of the plugin for the right reasons. Ideally our setup would remain as simple and out-of-the-box as possible. It's a good idea that perhaps we could try if we are at real loggerheads.

edit brevity..

DanVanAtta commented 5 years ago

@RoiEXLab @ron-murhammer

ron-murhammer commented 5 years ago

@DanVanAtta I tend to agree with either @ssoloff or @tvleavitt suggestions. Either configure what we have to wrap more aggressively or consider forking the google format plugin to have longer line length (or find something else). I really don't mind what we have now much so don't see a whole lot of reason/benefit to change.

My general opinion on why longer line length matters for this project vs say your average web app/service are:

  1. We still have a lot of legacy code that will not wrap very well
  2. We have a lot of complex code around delegates/rules and AI which lends itself towards long variable/method names
  3. Re-formatting the whole code base to a shorter length tends to end up with lots of strange results unless you allow joining of lines which then removes the effort folks already put in to formatting certain complex portions of the code.
DanVanAtta commented 5 years ago

@ron-murhammer I do not think we should be overly sad to remove the hand-formatting investment by fully-automating it. I'm concerned that we are in the meantime left to maintain and enhance that hand-formatting. It does not take too long to find examples where our custom line breaking falls short. We introduce further problems by leaving it up to the author to determine optimal breaking, and we spend review cycles even looking at format. The proposal to download and use a plug-in that removes all that effort and fixes everything is incredibly compelling IMO.

The custom eclipse formatter config option I think has too many nails in its coffin:

A custom fork of the plugin is perhaps viable. Though, I do not think that it is necessary or a very good decision. google-java-format configuration is immutable by design. I know that 120 vs 100 will majority impact commentary line lengths. I strongly do not like commentary breaking at 120, I don't even like it breaking at 100.

So it is compelling as well I think for us to use the built-in plugin:

To respond to this point specifically:

Re-formatting the whole code base to a shorter length tends to end up with lots of strange results

Agree that is the case for eclipse formatter, and that line joining produces some horrible results as well. Google-java-format is an intelligent, line-joining, nearly deterministic formatter. It is a plug-in and not a set of formatter rules for a reason.

IIRC, our current formatter rules are from google, which is a near-approximation to the real McCoy already. I clearly think it's high time we get the real McCoy, automate this, and profit.

ssoloff commented 5 years ago

It does not look like eclipse formatter is capable to conditionally break lines at the right places

As mentioned above, there's lots of configuration options:

eclipse-formatter-line-wrapping-settings

For example, for a series of chained method calls (e.g. coll.stream().filter(...).map(...).collect(...);) to be wrapped one call per line (if needed), you can just change Function Calls > Qualified invocations to Wrap all elements, except first element if not necessary.

remove unnecessary HTML

If you're talking about closing tags for block elements (e.g. </p>), I believe that's by design. Although, if you don't include closing tags for block elements, it won't add them.

or do HTML indentation

I believe the Javadoc formatter doesn't indent HTML block tags and their descendants because it is assumed devs will typically read the formatted Javadocs, not the unformatted Javadocs.

I strongly do not like commentary breaking at 120, I don't even like it breaking at 100.

Eclipse formatter can be configured to break comments at a different line length than code, if that's desired.

RoiEXLab commented 5 years ago

I'd agree with @ron-murhammer That legacy code is probably the biggest issue & the super long names are not great to work with, but I can see that in "newer code" this isn't so much of a problem. I really have mixed feelings about this. On one hand I kinda like the current formatting restrictions we have and don't really want to bother getting used tpto different standards (even if done automatically, it still looks weird). On the other hand I have used ESLint's auto-fix feature for the nodejs dice server recently and know what an improvement in efficiency it is to have line length issues etc. fixed automatically in a consistent manner.

DanVanAtta commented 5 years ago

It's interesting the auto-format file we got from google 2 years ago and have been maintaining since is currently so different from the plugin-in. I wonder if they were always that different, or if our file is out of date.

@RoiEXLab From what I've seen, final on arguments and number of args is more significant than long names. For example, with 3 arguments, it is an extra 18 characters, nearly 20% of the total line. Because of that, I've been personally drawing less distinction between old-code vs new-code.

Regardless, there is a cost to not making the cut-over:

My preferences for (mutually exclusive) options:

  1. we cut-over to the google-java-format plugin and configure it so all PRs are automatically formatted
  2. we simply cut over to google-java-format plugin and fail any PRs that are not formatted
  3. Option 2, but with a forked plugin set to 120
  4. Fail PR builds if eclipse formatter file has not been run, stop reviewing for formatting (if auto-format is run, no matter what-else, the formatting is good-enough)
  5. Simply stop reviewing for formatting, if checkstyle passes then it is good enough, spend no further effort on this area.
  6. Option 4, but drop our formatter file and overwrite with a fresh copy from google
  7. Option 4, and tweak our current eclipse formatter file to emulate google-java-format plugin
  8. Status quo, keep hand-formatting assisted by checked-in auto-format file, keep as a concern for reviewer that formatting is well done.
DanVanAtta commented 5 years ago

This is more salient than I thought, the following example is updated by formatter to cause a checkstyle violation:

/**
 * First line.
 *
 * <p>Second line
 */

Converted to the following and causes a checkstyle violation:

/**
 * First line.
 *
 * <p>
 * Second line
 */

This is actually pretty bad as something needs to be fixed, or else an auto-format of this file will introduce a build-breaking error.

I do not think we should not continue to saddle this project with formatter IDE custom configurations and continue to pay theses kinds of ongoing costs

DanVanAtta commented 5 years ago

I don't know why everyone else is not up in arms over hand formatting HTML in javadocs, or even hand-formatting in general and why we do not automate this. Are we a religious group, or programmatic? What is a path forward, is there a compromise here? More information needed or questions to be asked?

Practically speaking, if there is no path forward to fix this via automation, I'd personally look to removing checkstyle rules that make hand-formatting of javadocs a burden. The 'no' answer here again is a non-answer, fixing things via further manual configuration of rules IMO is missing the point of how we got here in the first place and what our objectives are.

DanVanAtta commented 5 years ago

@ron-murhammer I'd like to get some resolution on this, shall we take vote, use your BDFL power to make a choice here?

It seems we are choosing between:

Regardless of the choice we'll need to get used to some slightly new rules. I think we should be weighing ease of setup, maintenance, and configuration complexity.

DanVanAtta commented 5 years ago

Sigh, this is not the issue that is going to get the set of problems I'm seeing solved. I will re-formulate the problem and open a new issue instead.