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

Enable autoformatting #392

Merged
merged 20 commits into from
Mar 4, 2021
Merged

Conversation

bitwiseman
Copy link
Contributor

@bitwiseman bitwiseman commented Feb 20, 2021

Description

Formatting is a continuing issue during code reviews. Different users' IDE's have different opinions on what is correct formatting and those also differ from when various reviewers consider correct.

This change enables a base set of formatting and import ordering rules, automating the process of formatting code and enforcing that formatting. The setting I've used are somewhat arbitrary, but at least attempt to not completely torpedo the existing formatting.

This change uses the set.changelist property (set only in CI) to switch from applying formatting to checking formatting. On developer machines, the change will automatically apply formatting. In CI, formatting will be checked not updated.

Once this PR is merged, I will go back and merge existing PRs with https://github.com/bitwiseman/github-branch-source-plugin/tree/task/formatting-base, rebuild them, and then merge in master. This should result in few to no merge conflicts.

I am aware that this is controversial, so I've included a range of people as reviewers and am happy to have more weigh in. However, I need to be clear that I consider the lack of this to be technical organizational debt that adds one more hurdle to entry by new contributors and adds time to every PR review.

If this works here, I plan to apply it to other plugins, at least those I am a maintainer on.

@bitwiseman bitwiseman requested review from dwnusbaum and timja and removed request for dwnusbaum February 20, 2021 02:29
@bitwiseman bitwiseman force-pushed the task/formatting branch 4 times, most recently from 5debf05 to 4f613f6 Compare February 20, 2021 03:41
@bitwiseman bitwiseman marked this pull request as draft February 20, 2021 03:49
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

If it’s automated I really don’t care much about the specifics, it shouldn’t be controversial imo, much better to let automation handle it than have humans worrying about it

@car-roll
Copy link
Contributor

+1
definitely prefer this over an IDE accidentally autoformatting an entire class that gets pushed up. Not to mention feeling like some sort of style gatekeeper when looking over a review and seeing something that doesn't quite adhere to the coding convention of the particular plugin.

@alecharp
Copy link
Member

💯 I feel like any automation to normalize the code in a PR to the rest of the project code is a plus. This way we can really focus on what the code is doing rather than being distracted by how the code is written.

)
)
);
Matchers.allOf(instanceOf(OriginPullRequestDiscoveryTrait.class),
Copy link
Member

Choose a reason for hiding this comment

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

Here the auto formatting has made it less readable than before.

Choose a reason for hiding this comment

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

Agree, we should get used to it, I guess

Matchers.allOf(instanceOf(OriginPullRequestDiscoveryTrait.class),
hasProperty("strategyId", is(2))),
Matchers.allOf(instanceOf(ForkPullRequestDiscoveryTrait.class),
hasProperty("strategyId", is(2)))));
Copy link
Member

Choose a reason for hiding this comment

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

Here it's very bad imho :)

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Helpful in some spots but personally I find this a net negative in terms of legibility. Mechanical formatting is just terrible at deciding where to insert line breaks.

Any change like this implies a “diff wall”: for the promise of reduced merge conflicts, review overhead, etc. going forward, you pay the cost of git blame becoming much less useful for understanding older code, and cherry-picking changes to backport branches preceding the cutoff becomes much harder.

import org.jenkinsci.plugins.github.config.GitHubServerConfig;
import org.kohsuke.github.GHRateLimit;
import org.kohsuke.github.GitHub;
import org.kohsuke.github.RateLimitChecker;
Copy link
Member

Choose a reason for hiding this comment

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

Switched to a simple alphabetical order IIUC? Very welcome. Humans should not be reading import statements to begin with; normalizing the order minimizes the chance of stupid merge conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

humans do code review.. whilst you may not read them and you may feel they are unwanted others like me find them very useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jglick
It is my understanding that what code style guidelines there are for Jenkins say "Simple alphabetical non-static, then simple alphabetical static". Is that incorrect?

@jtnord
What you prefer the ordering be? I don't care what it is as long as I never have to tell someone in a code review to change it.

Copy link
Member

Choose a reason for hiding this comment

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

So long as it is simple I would not really care. (Whether static imports should be allowed at all, and under what conditions, is a contentious topic.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jglick
The topic of static imports will not be solved by a formatter.

Copy link
Member

Choose a reason for hiding this comment

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

What you prefer the ordering be? I don't care what it is as long as I never have to tell someone in a code review to change it.

I have personal preferences but I do not care too much about the order apart from always non static before static imports though, and static imports should be reserved for test code apart from those very rare special occasions.

But given as you asked, my preference is someone along the following lines.

  1. core libraries first (java, javax)
  2. then 3rd party libraries (com .edu , org)
  3. then a break
  4. then jenkins core code (hudson, jenkins, org.jenkinsci, io.jenkins)
  5. then stuff from my own project (e.g. org.jenkinsci.plugins.github_branch_source)
  6. then a break
  7. then the same but for static imports.

That said my IDE will automatically an import put it where it is configured to by its ordering (eclipse) and having per project settings is a PITA (I do not recall the last time I typed an import statement). I try to reduce diff bloat it after the fact by manually moving it if I recall :)

The topic of static imports will not be solved by a formatter.

you mean the ordering or that they should or should not be allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whether or not they should be allowed.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW my personal preference is strictly alphabetical, no line breaks, no attempt to group in any way other than | sort.

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

Personally I have never met an autoformatter that works in all cases. so this leads me to ask
what are the equivalent //@autoformat:off //@autoformat:on flags and will it make the code more or less useable.

I also dislike things that change code just because I build, I like to know what happens - sure let the build fail if I need to run mvn -Pfix-the-formatting validate then let it be so.

import org.jenkinsci.plugins.github.config.GitHubServerConfig;
import org.kohsuke.github.GHRateLimit;
import org.kohsuke.github.GitHub;
import org.kohsuke.github.RateLimitChecker;
Copy link
Member

Choose a reason for hiding this comment

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

humans do code review.. whilst you may not read them and you may feel they are unwanted others like me find them very useful.

Comment on lines 28 to 29
long checkRateLimitImpl(@NonNull GHRateLimit.Record rateLimit, long count, long now)
throws InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

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

I would say this is less readable than before.

Suggested change
long checkRateLimitImpl(@NonNull GHRateLimit.Record rateLimit, long count, long now)
throws InterruptedException {
long checkRateLimitImpl(@NonNull GHRateLimit.Record rateLimit,
long count, long now) throws InterruptedException {

is more readable to me .. but 🤷 you choose somethign and it gives you something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see the argument that throws should go on the same line. But whether this less readable is debatable - do you really want to debate it?

Copy link
Member

Choose a reason for hiding this comment

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

you can choose to disregard my opinion here and I am ok with that.
hence

🤷 you choose something and it gives you something.

auto formatters will always produce something that is worse. this is to me one (all be it a very minor) case of that.

@@ -96,12 +98,14 @@ public LocalChecker getChecker(@NonNull TaskListener listener, String apiUrl) {
if (GitHubServerConfig.GITHUB_URL.equals(apiUrl)) {
// If the GitHub public API is being used, this will fallback to ThrottleOnOver
LocalChecker checker = ThrottleOnOver.getChecker(listener, apiUrl);
checker.writeLog("GitHub throttling is disabled, which is not allowed for public GitHub usage, so ThrottleOnOver will be used instead. To configure a different rate limiting strategy, go to \"GitHub API usage\" under \"Configure System\" in the Jenkins settings.");
checker.writeLog(
Copy link
Member

Choose a reason for hiding this comment

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

should be aligned to writeLog( not checker.

basically are you using a fixed offset for wrapped indentation? (ie 8 which is the lengh of checker. - in which case don't use an offset that matches to the thing you are offsetting.

otherwise you can see something like

checkz(.writeLog(
            "a very long string),
            "and something else")

and that is worse than evil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtnord
Same debatable point as above. The indent you are saying is missing was (as far as I can tell) intentionally removed as redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtnord
Regarding the align to column, you can change this the formatter as noted here to be 82 and it will align to column, but it produces results that I think you'll like even less than the current results. 50 is also an option, but it ends up even worse.

But maybe I'm misunderstanding what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

so given the settings that are being used are offset by a specific amount (and not column) if it hard to see when something is wrapped in something like

foo(bar(wibble(baz, bob,
        jennygoestomarket),
        markstaysathome));

Because both jennygoestomarket and markstaysathome are offset by the same amount, and when scanning the code it looks like they can be the arguments to the same function (when they are not).
Whitespace is helpful to show what things apply to (which is why we indent blocks {) to me the same applies here.
Maybe I misunderstand the settings you are applying? (but I have had many issues with eclipse when using this setting in the past)

disclaimer #1 - I much prefer projects that are set to indent on column.
disclaimer #2 I am mildly dyslexic and so this is important to me. My brain scans and reads text in a specific and different to the vast majority of people.

now - I am not a major contributor to this project and so I am not trying to enforce my standards onto the people who do work with it much more than I do (I have only had a few minor contributions) and as such I will adapt when I need to work in this project. However if this is the first step into trying to get something more standard into Jenkins ecosystem and rolling out more widely then being inclusive of all people (which includes the neuro diverse) should be taken into account, as descisions made here can make it more or less easy for people to contribute.

disclaimer #3 on my projects when I remember to switch to my coding style I indent with tabs so that people can customize their indent view to support their needs (2 for some people 8 for others, and some have 4 or even 3) but offset with spaces. this allows the code to look the same regardless of tab space setting and allow people to have a visual offset that conforms to the way they work best.

and finally disclaimer #4 all attempts at auto formatting will break somewhere. This is why I think auto formatters are evil. let the bot do a formatting review, let a human decide to reject it's findings, or to easily make it better. See the end part on disclaimer 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtnord

  1. The problem I've seen with with indenting to column is you end up with deeply indented code. I assume your suggestion for addressing that is to refactor the code into smaller methods, avoiding deeply nested structures. Am I correct?

  2. Neuro-atypicalness and accessibility is certainly a valid concern. I read slowly and also depend on formatting to allow me to take in code at a reasonable speed. I definitely want to be sensitive to a broad range of contributors needs. At the same time, consider that someone else might find "align to column" harder to read due to its deeper indenting causing lines to wrap more often. Which one wins? In the case of this change, I am looking to apply a standard formatting that most closely matches what is already being used. There are parts of this code base that seem to be using "align to column", but the majority of it does not.

  3. I believe this mix of tabs and spaces is referred to as "smart-tabs". It works best in "align to column" if I understand it correctly. I'm not opposed to this, but as with point two it seems a larger change. I've done like I did with no-join and created a branch using this and "align to column" settings: bitwiseman:task:task/formatting...bitwiseman:task/formatting-columns. Perhaps there's other settings that could be applied to do this better. Perhaps you as the one bringing this up could try various setting to find one that is best in your opinion?

  4. I would instead say that all attempts at formatting will be imperfect, both humans and automated systems. Auto-formatters are generally imperfect in rigid and predictable ways, whereas human formatters are imperfect in a wide range of random ways. I categorically disagree that humans should be allowed to opt-out of auto-formatting - given the option humans will choose to do things their own way and then we end up talking about "better" or "worse" formatting instead of focusing on making the code better.

Copy link
Member

Choose a reason for hiding this comment

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

My preference would be to use the autoformatter for things it can do reliably—sort imports, remove trailing whitespace, enforce spaces vs. tabs for block indentation, normalize use of spaces around / inside tokens like ( ) =—but for more subjective things like placement of line breaks leave the choice to humans.

Comment on lines 94 to 96
public ForkPullRequestDiscoveryTrait(
int strategyId,
@NonNull SCMHeadAuthority<? super GitHubSCMSourceRequest, ? extends ChangeRequestSCMHead2, ? extends SCMRevision> trust) {
Copy link
Member

Choose a reason for hiding this comment

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

worse than before by a long way.
see previous comment about indention based on a fixed amount vs relative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtnord
Debatable. Do you really want to debate this?

@jglick
Copy link
Member

jglick commented Feb 22, 2021

I also dislike things that change code just because I build, I like to know what happens - sure let the build fail if I need to run mvn -Pfix-the-formatting validate then let it be so.

💯, principle of least surprise says that commands like mvn clean verify should never modify my working tree (or leave untracked files).

I feel like there was some detailed proposal for formatting enforcement a year or two ago which acknowledged this, but not sure where that was. Cannot find a JEP on the topic. Maybe some ML thread?

@jglick
Copy link
Member

jglick commented Feb 22, 2021

@jtnord
Copy link
Member

jtnord commented Feb 22, 2021

I've come to the conclusion that standards are best enforced as review comments from a bot in a PR.

This way the code owner can disregard it where it is less readable, and as it comes from a bot it is not personal.

Provide me with an easy way to know how to format the code (which could be an explicit maven goal or a profile/phase)

I think the config-as-code plugin does that (may be some other repo I am mixing it up with)

When the @see method starts with a '<', it must end with the same.
@bitwiseman
Copy link
Contributor Author

bitwiseman commented Feb 23, 2021

@rsandell
Out of curiosity regarding the less than ideal formatting you pointed out, I tried adding "join wrapped lines = false" to the settings. It addressed this issue and also some of the objections from @jglick (array initialization on multiple lines no longer get collapsed). However, the result is significantly less consistent and more prone to odd hanging clauses - the user added a new line somewhere at some point and it just stays there.

You can see the result here:
https://github.com/jenkinsci/github-branch-source-plugin/compare/master...bitwiseman:task/formatting-no-join?expand=1

@bitwiseman bitwiseman marked this pull request as ready for review February 24, 2021 03:29
Applying spotless formatting is very easy, "mvn spotless:apply".
Instead of running apply automatically, we leave it to the user to run it manually
before submit committing/submitting a PR.  However, "mvn install" will fail if
spotless:check doesn't pass.

This seems a reasonable compromise to the behavior discussed.
@bitwiseman
Copy link
Contributor Author

bitwiseman commented Feb 24, 2021

@jglick @jtnord @rsandell
I really appreciate your feedback on things that looked bad. I have updated the setting to address that feedback where possible.

There's a ton to unpack here. And the volume of topics to unpack is where PR's like this often die. There's a sufficient volume of objections and points of contention that the change does not get approved.

The counter example is github-api. I enabled this system on github-api and it has resulted in my never having to tell anyone to fix their formatting. I don't even have to consider if I need to suggest a different formatting because they might have chosen to ignore the suggested formatting. This has allowed everyone to focus on implementing features and fixes resulting in more contributions from a broader range of people. Jesse did indeed find some cases where the formatting was not great, but I think that is indicative of the API being used being rather clunky as well.

I've come to the conclusion that standards are best enforced as review comments from a bot in a PR.

This way the code owner can disregard it where it is less readable, and as it comes from a bot it is not personal.

And I've come to the conclusion that my time is my most scarce resource, and I don't want to spend any time dealing with different contributors opinions of what is "less readable".

@jtnord

Personally I have never met an autoformatter that works in all cases. so this leads me to ask
what are the equivalent //@autoformat:off //@autoformat:on flags and will it make the code more or less useable.

There is an option to allow turning the formatter off and on. I've left it disabled for now. If a maintainer steps up that wants to enable this setting, I will gladly step aside and let them take over this plugin.

I also dislike things that change code just because I build, I like to know what happens - sure let the build fail if I need to run mvn -Pfix-the-formatting validate then let it be so.

💯, principle of least surprise says that commands like mvn clean verify should never modify my working tree (or leave untracked files).

From a build correctness perspective, we should not modify files during the build. I've changed the behavior to check by default and the user can manually run spotless:apply when they are ready.

Any change like this implies a “diff wall”: for the promise of reduced merge conflicts, review overhead, etc. going forward, you pay the cost of git blame becoming much less useful for understanding older code, and cherry-picking changes to backport branches preceding the cutoff becomes much harder.

Yes, the "diff wall" is real and is a cost. We will pay the cost now and be done with.

@bitwiseman
Copy link
Contributor Author

bitwiseman commented Feb 26, 2021

From the email list:

I've updated #392 with the following changes:

  • Source will not be changed during the build unless the user runs a specific maven goal
    • "spotless:check" runs near the end of the lifecycle during the verify phase during local build. Formatting issues will fail the build, source remains unchanged.
    • "spotless:check" runs near the start of the lifecycle during the process-sources phase during CI. Formatting issues will fail the build, source remains unchanged.
    • Users may run "mvn spotless:apply" to automatically fix formatting. This command is easy and short. When "spotless:check" fails it specifically tells users to run "mvn spotless:apply" to fix formatting.
  • I have adjusted the formatting rules to address some of the points raised during review
    • Line comments do not join if wrapped - if you have comments that you want to not be reformatted, keep them under 120 chars and use line comments
    • Javadoc formatting made a bit more compact - ea6e28c
    • Found a way to force array initializer expansion if that is what is desired - 43a9950
    • Long string arguments getting pushed to the next line? Maybe the humans involved should split those up. Enable autoformatting #392 (comment)
    • I've applied the ".gitattributes" suggested by Joseph. The build now passes on Windows.
  • I have also not addressed a number of other places where people have said the formatting looks "worse". These cases are generally debatable/personal-preference. In some of these cases I agree with the assessment but I don't see it as a blocker. If anyone disagrees with that evaluation, feel free to say so and explain why.

Note: I am using the eclipse formatter rather than to google formatter in an attempt to reduce the severity of the diff wall and disruption to folks familiar with the current code style. There are still significant differences, but they are far less than switching to google formatting. However, that is still an option. You can see what it would look like here: https://github.com/bitwiseman/github-branch-source-plugin/pull/new/task/formatting-google

@basil has made a strong case for switching to Google Formatter, which would obviates my tweaks above. If I follow my own stated reasoning to its logical conclusion, what's he's saying makes sense. On the other hand, the "diff wall" in that case is absolute. I need to sit with this for a day or two, but I will likely make that change.

@jtnord
Copy link
Member

jtnord commented Feb 26, 2021 via email

@jglick
Copy link
Member

jglick commented Feb 26, 2021

you just may have to look at the parent of the diff that changed the line - but that is still the case today - e.g. refactoring

Sure, this just means you would need to do that in basically every case, rather than on occasion.

I agree with @basil’s reasoning about Google Format. As to the actual formatting it produces, it seems comparable to any other machine formatting I have seen—i.e., loses human intent and is somewhat less legible, and the line length seems artificially short, but 🤷 tolerable.

(Obviously I only rarely touch this particular repo any more, so I am just kibitzing.)

@bitwiseman bitwiseman mentioned this pull request Feb 26, 2021
8 tasks
@bitwiseman
Copy link
Contributor Author

bitwiseman commented Mar 3, 2021

I have chosen to the Google Formatter specifically because it doesn't provide a bunch of settings. We apply the formatting and that is it.

I've merged the fixes needed before formatting into the master branch already to minimize merge conflicts later.

This change will be a squash merge to keep the diff wall to one commit.

@bitwiseman bitwiseman merged commit d3e3216 into jenkinsci:master Mar 4, 2021
bitwiseman added a commit that referenced this pull request Mar 4, 2021
This change turns on auto formatting using the Google Format for Java tool.

This change runs checks during the build and will fail the build during the verify phase if the
formatting checks fail.  The failure message tells users to run `spotless:apply` to fix formatting.
After running `spotless:apply` the formatting will be correct.

This is the squashed commit for #392 #392 .
Due to my clicking the wrong button, I did a merge rather than a squash in the PR.
This change causes a "diff wall", so squashing is the better option to make the depth of that wall only one commit.
@jglick
Copy link
Member

jglick commented Mar 24, 2022

Diff wall is partly ameliorated by #528. This only works in GitHub view, unless you configure other tools to read the same file; and only helps with blame, not other things like refreshing old PRs, though in the medium-to-long term it is blame which matters most.

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.

8 participants