[proposal] coding style for core (jenkins 2.0?)

[Kanstantsin Shautsou]

Every time i open core code to fix something my eyes become red because of unreadable code. The same i heard many times as bad argument for jenkins on conferences.

This doesn't cause any backward compatibility issues and cherry-pick shouldn't be a problem for LTS because near 2.0 LTS i think will be handled in different way.

My proposal is to chose some coding style and follow it in future.

"Any code style" rule today led to hardly readable code:

- Mess of spaces vs tabs: bad diffs

- Annotation mess: when i'm reading code i expect access modifiers to be as keywords on start of line. Project may have different annotations and eyes can't jump to everybody known keywords.

- Line width: scrolling code extremely inconvenient (imho even 120 sometimes not enough, but producing >150 enforces scrolling). 

- Random spaces around if/for, looks like some developers coded from calculators. All IDEs can auto format code (i also lasy to type spaces in right places, but i usually auto-reformat before commits).

- {} braces for if/for bodies: enough to make bug one time to understand.

Many experienced developers already added rules in their plugins and imho for core something neutral can be chosen. Imho oracle coding style is the most used and can be adopted. For example Stephen C. already has documented variant that mostly matches Oracle code style.

- Code will be more attractive for newcomers.

- Will allow automate checks. 

WDYT?

[Mark Waite]

I'm a proponent of automated code formatting.  If we're describing a coding style in the "CONTRIBUTING" file, why not let a tool enforce that coding style on each compile of the code?

Automated code formatting has the down-side that there is a one-time "diff wall" which is created when the automated formatting is introduced.  I think the time savings later in maintainers not having to enforce the coding style by inspection (read and remind about white space changes, complain about missing braces, etc.) easily repays the cost of the "diff wall".

Mark Waite

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/06b5602c-2f6a-4cbe-91ab-bfdb23f3f2c4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Arnaud Héritier]

+1 I'm "praying" for documented coding rules in Jenkins for so many years.

I don't want to enforce anything but at least to have them to:

* allow contributors to follow them if they can/want (and they should like to follow them to propose readable PRs which are easily accepted)

* allow maintainers to reformat code of contributions without asking anything

If the IDE configuration if provided at least for eclipse and IJ it will cover a large part of the developers and it won't be a nightmare each time you are opening a file and for a simple change you need to guess how the code is formatted.

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/06b5602c-2f6a-4cbe-91ab-bfdb23f3f2c4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

-----

Arnaud Héritier

http://aheritier.net

Mail/GTalk: aheritier AT gmail DOT com

Twitter/Skype : aheritier

[nicolas de loof]

I have to admit gofmt make it far simpler to contribute to go projects as nobody has to discuss code formatting rules. Enforcing the expected format is applied everywhere make it simpler. So +1 on applying such a formater to all code and apply automatically as part of PR review to ensure it has been followed.

That being said, I sometime get red eyes looking at Jenkins code base but this is not due to line indent or missing spaces before a bracket, but due to hundred @deprecated and obsolete libs being used :P

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAFNCU-9pt%3DngEBMHTHSiO1nR6SK6rB8ycP2-HsgQJBXt%2B1EmHA%40mail.gmail.com.

[Kanstantsin Shautsou]

Example of sonar + github automatic integration https://github.com/jenkinsci/envinject-plugin/pull/73#issuecomment-143682168 . Such checks allowed not bother ourself with boring commenting on github-plugin/github-pullrequest-plugin

[Kirill Merkushev]

example moderate checkstyle:

https://gist.github.com/lanwen/96feea5efdbb4ee86cad

понедельник, 26 октября 2015 г., 18:24:19 UTC+3 пользователь Kanstantsin Shautsou написал:

[Jesse Glick]

-0 I guess. I have never had any problem understanding anyone else’s
code because of their formatting choices, weird or not. It is the
logic that is the problem. Enforced code style, like discussions about
code style, has just been a distraction and waste of time in my
experience.

If you are going to set a coding standard, it must be enforced
mechanically by a standard Maven build, so contributors can fall into
line on their own time, without bothering reviewers.

And I think the “diff wall” is potentially a drag for years to come. I
still look through line-by-line history for the reasoning behind
historical decisions, sometimes going into imported Subversion history
(which alas is sometimes where the investigation ends, due to
Subversion’s poor merge tracking). Every time a line is changed for no
good reason, understanding the subtle and undertested assumptions in
Jenkins code becomes a little bit harder.

[Nigel Magnay]

-1.

Automated code formatting in general makes a total pigs breakfast of fluent APIs. 

I find it hard to get inside the heads of people that love to spend time worrying about how many spaces come before and after the brackets in an "if" statement. Sure, have some guidelines, make sure you don't do things like hungarian notation that add no value - but most arguments I've seen boil down to an 'appeal to authority'.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtG6XBYLGo8%3DE1dFEXMMY6ivV4qHzm%3DQhW3_pNOrm1JcxA%40mail.gmail.com.

[Kanstantsin Shautsou]

On Monday, October 26, 2015 at 7:01:07 PM UTC+3, Jesse Glick wrote:

-0 I guess. I have never had any problem understanding anyone else’s
code because of their formatting choices, weird or not. It is the
logic that is the problem. Enforced code style, like discussions about
code style, has just been a distraction and waste of time in my
experience.

Because you know very well this code base and you do your own styling that mistakenly copied by other newcomers and ends in mess of styles. 

That's why automation check should save time and enhance quality. 

If you are going to set a coding standard, it must be enforced
mechanically by a standard Maven build, so contributors can fall into
line on their own time, without bothering reviewers.

See example of automated check https://github.com/jenkinsci/envinject-plugin/pull/73#discussion_r40529580 . I think maven checkstyle can't check diffs, so without reformatting all code this wouldn't work. That's why i proposed "apply for future changes" because new APIs getting old style randomness. 

And I think the “diff wall” is potentially a drag for years to come. I
still look through line-by-line history for the reasoning behind
historical decisions, sometimes going into imported Subversion history
(which alas is sometimes where the investigation ends, due to
Subversion’s poor merge tracking). Every time a line is changed for no
good reason, understanding the subtle and undertested assumptions in
Jenkins code becomes a little bit harder.

Strange that unsquashed PRs with 20 'fix typo' commits is not a problem.

[Jesse Glick]

On Mon, Oct 26, 2015 at 12:44 PM, Kanstantsin Shautsou
<kanstan...@gmail.com> wrote:
>> I have never had any problem understanding anyone else’s code because of their formatting choices
>

> Because you know very well this code base

How do you think I got to know (some of) the code base in the first
place? By fixing problems, and digging through history to see when and
why those problems were introduced.

> See example of automated check
> https://github.com/jenkinsci/envinject-plugin/pull/73#discussion_r40529580

Interesting, though sounds very painful to contribute to since you
must first file the PR, then go back and wait for the bot to ask you
to reformat. Much better to be able to repeatedly run a style checker
tool locally before committing. Not sure if there is any such tool
which is able to automatically skip lines already present in the
`master` branch or something like that.

[Kanstantsin Shautsou]

On Oct 26, 2015, at 21:01, Jesse Glick <jgl...@cloudbees.com> wrote:

On Mon, Oct 26, 2015 at 12:44 PM, Kanstantsin Shautsou
<kanstan...@gmail.com> wrote:

I have never had any problem understanding anyone else’s code because of their formatting choices

Because you know very well this code base

How do you think I got to know (some of) the code base in the first
place? By fixing problems, and digging through history to see when and
why those problems were introduced.

I do the same, but last year unsquashed+normal PRs + more random made everything worth.

See example of automated check
https://github.com/jenkinsci/envinject-plugin/pull/73#discussion_r40529580

Interesting, though sounds very painful to contribute to since you
must first file the PR, then go back and wait for the bot to ask you
to reformat. Much better to be able to repeatedly run a style checker
tool locally before committing. Not sure if there is any such tool
which is able to automatically skip lines already present in the
`master` branch or something like that.

- IDEA autoformat can be applied to selected block of code https://www.jetbrains.com/idea/help/code-style-and-formatting.html (not exactly what you want, but sometimes helps)

- I don’t know any PRs that was accepted without changes. Default formatted code usually fits into the most conservative styling.

- Rules can exist in checkstyle and be applied by any tool when it will supported.

- Logically only during PR you can do all changes because after, `diff wall` wouldn’t allow to touch committed code. It should not be so difficult to fixup commits.

- The same pain as Update Centre filtering for plugins that doesn’t have link to doc. You should release plugin, wait for metadata updates, debug all chain and only then understand what happened. But such not obvious flow was accepted for all 1k plugins. It even can’t say to email that something wrong.

- Local run will require huge amount of time and people always forgetting to run checks. PR is the only place where we can point to such things.

- Also comments in PR can be ignored ;)

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/8fjvXGYbFJ4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr2W0x5Q2NTAaZMQz4G5nvP9-6XTLKbTxP9VkFSvQML%2BvA%40mail.gmail.com.

[Stephen Connolly]

I think that the best way to do this is via an experiment in plugins... If we get a critical mass of plugins adopting a mostly similar set of rules then and only then should we think about applying them to core

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/D4B215A1-C73A-44A1-A04E-D9195C1F9325%40gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
Sent from my phone

[Nigel Magnay]

> That's why automation check should save time and enhance quality. 

It only “enhances quality” if you agree with the idea that a set of arbitrary syntactic checkstyle rules spat out by tooling represents “quality” rather than merely “conformity". The truth is ‘quality’ is rather hard to measure - particularly in an automated way - and we’re left with the old management adage that those who can’t measure what they want, end up wanting what they can measure. This is why poor managers often clock-watch and obsess as to whether their staff turn up in a suit & tie or not.

e.g: JDK7 has, according to SonarQube, over 100,000 “Major, Critical or Blocking” issues.  IMO, that says much more about the value of SonarQube than the quality of the JDK (which, incidentally, has an almost entirely uniform code style).

If a PR ‘fails’ from a bunch of checkstyle rules, you’re effectively asking the original author to expend additional effort on ‘conformity’. The time cost delivers zero end-user value, and meanwhile you’re piling up inventory — risking yet more rework in the event of a merge conflict, or the author simply giving up (after all, he’s given this code to you for free). Software development these days claims to be all about being agile - and yet the above is practically the ‘lean’ textbook example of waste.

Having a clean history - not having to dig through commits that did nothing to change the semantics (merely dicked about with the whitespace) - turns out to be far more important now that tooling has moved to the state when histories become actually useful. Which is why ideas from old-school coding standards (like making variable names line up horizontally  - which actually really does improve readability) are basically dropped (because this makes a single character change potentially ripple to several unrelated lines, polluting the history and potentially making merges harder).

[Kanstantsin Shautsou]

That leads to situation that people hacking things in plugins instead of extending/fixing core. Plugins usually copy-pasting examples from core (personally i copied as is jelly hacks and protected methods like getDefaultParamaeters from JobMixin) and that situation goes into loop. Imho core should be example for all project and not vice versa.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMx93kpHmwJyjLmBe-pmKTzC5haH%3DbGrca-3UqZMfegiug%40mail.gmail.com.

[Stephen Connolly]

And `git blame --first-parent -w` becomes much more important when
tracking and tracing changes...

Once you start down the `--first-parent` route you then realize that
all this fussing over squashing commits is a waste of time, there is
no need to squash commits... the only need is to fix the GitHub UI so
that people who are not CLI ninjas can gain the benefit...

Please everyone start pestering GitHub to add the support for
--first-parent and -w on the git blame and git log web pages... if
it's just my request sitting in their in box it may take a while... if
everyone asks for it... well they should fix it faster.

>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to jenkinsci-de...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/jenkinsci-dev/CAPYP83SVJT7d6TN07Xh_obV94Y09n84Mtb8T6u6gyZAZ%2BEHS%3Dw%40mail.gmail.com.

[Kanstantsin Shautsou]

On Oct 26, 2015, at 22:34, Nigel Magnay <nigel....@gmail.com> wrote:

> That's why automation check should save time and enhance quality. 

It only “enhances quality” if you agree with the idea that a set of arbitrary syntactic checkstyle rules spat out by tooling represents “quality” rather than merely “conformity". The truth is ‘quality’ is rather hard to measure - particularly in an automated way - and we’re left with the old management adage that those who can’t measure what they want, end up wanting what they can measure. This is why poor managers often clock-watch and obsess as to whether their staff turn up in a suit & tie or not.

Sorry, i’m not a master in English. I sure that such things will finally end with quality (FB analyser attempt was successful?).

e.g: JDK7 has, according to SonarQube, over 100,000 “Major, Critical or Blocking” issues.  IMO, that says much more about the value of SonarQube than the quality of the JDK (which, incidentally, has an almost entirely uniform code style).

Feel free to investigate why they have so many “issues”, whether they tuned default profiles and etc.

If a PR ‘fails’ from a bunch of checkstyle rules, you’re effectively asking the original author to expend additional effort on ‘conformity’.

This is consistency and readability.

The time cost delivers zero end-user value, and meanwhile you’re piling up inventory — risking yet more rework in the event of a merge conflict, or the author simply giving up (after all, he’s given this code to you for free).

I will repeat answer that i said personally to you some time ago. When i see that contributor is newbie and can’t solve something i’m picking his changes with rework. That’s what Oleg do in core when PR author can’t do some changes and disappear. Note that happens always and not because of styling changes.  If you can’t make such primitive thing, then it indicator that you may fail in code.

 Software development these days claims to be all about being agile - and yet the above is practically the ‘lean’ textbook example of waste.

I know your methodology: don’t use static analysers, don’t do changes with PRs, don’t fix problems in upstream, do hacks, no tests.

Having a clean history - not having to dig through commits that did nothing to change the semantics (merely dicked about with the whitespace) - turns out to be far more important now that tooling has moved to the state when histories become actually useful. Which is why ideas from old-school coding standards (like making variable names line up horizontally  - which actually really does improve readability) are basically dropped (because this makes a single character change potentially ripple to several unrelated lines, polluting the history and potentially making merges harder).

JDK code is very well documented and readable.
Interesting, how do you dig core history with not squashed PRs today (but it parallel thread).

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/8fjvXGYbFJ4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPYP83SVJT7d6TN07Xh_obV94Y09n84Mtb8T6u6gyZAZ%2BEHS%3Dw%40mail.gmail.com.

[Mark Waite]

On Mon, Oct 26, 2015 at 1:21 PM Stephen Connolly <stephen.al...@gmail.com> wrote:

I think that the best way to do this is via an experiment in plugins... If we get a critical mass of plugins adopting a mostly similar set of rules then and only then should we think about applying them to core

I volunteer to experiment with a branch of the git client plugin as a first candidate.  I'd limit the formatting to newer files and files that I've created myself so that the "diff wall" won't be as difficult.  Older files with wildly divergent formatting styles will remain that way as part of the first phase of the experiment.

 

That will give a chance to evaluate Nigel's concern for the impact on fluent API calls (since there are several fluent interfaces in the git client plugin).

Mark Waite

[Nigel Magnay]

You don't need to trial automatic code formatting it to know it's going to produce a terrible result.

Trivial example 101. Which is the more easily parseable to the human eye?

periodFormatter = new PeriodFormatterBuilder()

                .printZeroAlways()

                .appendDays().appendSuffix("d ")

                .appendHours().appendSuffix("h ")

                .appendMinutes().appendSuffix("m")

                .toFormatter();

or

periodFormatter = new PeriodFormatterBuilder().printZeroAlways().appendDays()

                          .appendSuffix("d ").appendHours().appendSuffix("h ")

                          .appendMinutes().appendSuffix("m").toFormatter();

I know which I'd rather be faced with when maintaining code.

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtHbxMpH1RTwRMiYuDj71pVi1fTSsUFbHTpEh%3DeYGMNezg%40mail.gmail.com.

[Kanstantsin Shautsou]

Both will be acceptable in styling. That lines are about logic that analysing tool can’t know.

or 

periodFormatter = new PeriodFormatterBuilder().printZeroAlways().appendDays().appendSuffix("d ").appendHours().appendSuffix("h ")                        .appendMinutes().appendSuffix("m").toFormatter();

<——— scroll ——---->

P.S. Am i alone who receives emails from Nigel with small blue font?

You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/8fjvXGYbFJ4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPYP83TJO306gyDmEt_fVx88XVZK54gK7O3JhxqSFzDgLrn6Xg%40mail.gmail.com.

[Nigel Magnay]

Both will be acceptable in styling. That lines are about logic that analysing tool can’t know.

​Bingo.

"Automatic code formatters"​ don't know the semantics of what they're formatting, so you apply them end up with gigantic concatented lines (sometimes split to 80 columns, as if we're still producing punched cards). 

This is clearly worse than the "unformatted" case - where the author has carefully aided the maintainer by splitting on semantically distinct sections.

By all means, have some IDE defaults that can be picked up (e.g: spaces or tabs, tabstops, whether to use '*' imports or not), but auto-code formatters are evil.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/FF20FA21-AF8B-46EE-B8DF-10A9DEBA8921%40gmail.com.

[Mark Waite]

On Mon, Oct 26, 2015 at 4:24 PM Nigel Magnay <nigel....@gmail.com> wrote:

Both will be acceptable in styling. That lines are about logic that analysing tool can’t know.

​Bingo.

"Automatic code formatters"​ don't know the semantics of what they're formatting, so you apply them end up with gigantic concatented lines (sometimes split to 80 columns, as if we're still producing punched cards). 

This is clearly worse than the "unformatted" case - where the author has carefully aided the maintainer by splitting on semantically distinct sections.

By all means, have some IDE defaults that can be picked up (e.g: spaces or tabs, tabstops, whether to use '*' imports or not), but auto-code formatters are evil.

You describe things with a very broad brush.  Earlier you extrapolated from a request to format code into a comment about management measuring the irrelevant.  Now you're declaring that the benefits my team of 10+ (using extreme programming as our methodology, pair programming everything, test driven development, etc.) found from automated code formatting over multiple years of working together should be ignored because "auto-code formatters are evil".  The sole specific example you offer is that calls to fluent API's aren't we'll preserved by automatic code formatters.

Can you offer other concrete examples of cases (preferably in the existing code) where an automatic formatter will have a serious negative impact?

When I look at the git-client-plugin source code, I find relatively few cases of the fluent calls being badly damaged by an automatic formatter.  There are many fluent calls, but most of them will be untouched by an automatic formatter.

There is a trade-off between the simplicity of automatically maintained code format (with occasional sub-optimal formatting as a result of the automation) and the work I must do on every pull request to not make code formatting consistency worse in the two plugins I maintain.

I only maintain two plugins of the 1000+, so I accept that I should not be considered as a major influence in the final decision.

Mark Waite

 

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPYP83Q73VNYKWJWX2iP_0QA9z%3Dy%3DpbDPBHMy0tQJ%2BX6Ley1Ng%40mail.gmail.com.

[domi]

I think formatting is a must and I would have loved to have such rules when I started to work on jenkins some years ago. Personally I think that the longer we wait to introduce this, the more it will hurt.

I’m happy to volunteer in the adoption of such formatting rules for my plugins - but I would love to have them defined in a central place.

Domi

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtHxPmXuDzResjM2PZx04p94khB55O2n4auEoUf9wdY_sA%40mail.gmail.com.

[Baptiste Mathus]

+0.9 though I agree this is generally a subject many people love to disagree on. I prefer having an automated codestyle people can just import into their IDE and just apply before committing/at save.

For example, the Maven dev team provides an xml export of the formatting rules for Eclipse[1], and I've used it in the past to contribute. 

Contrary to what some people seem to think, I actually liked and still like it: IMO it makes it easier for newcomers to contribute. 

Personnally my main side-concern (apart from the code itself I mean) when first contributing to an OSS project is respecting its rules. 

Having an automated formatter available makes that part a no-brainer and ppl can concentrate on the real issues.

As for the rare cases where you want to manually format code for the sake of readability for whatever reasons, every formaters out there generally support tags to disable/reenable automated formated locally [2].

[1] https://maven.apache.org/developers/conventions/code.html#Eclipse_3.2

[2] example with Eclipse http://batmat.github.io/presentations/50-slides-of-ide/prez.html#formatterOnOff

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/D7807A3C-0F85-4CD8-A1DB-6E907B2CCA90%40fortysix.ch.

For more options, visit https://groups.google.com/d/optout.

--

Baptiste <Batmat> MATHUS - http://batmat.net
Sauvez un arbre,
Mangez un castor !

[Baptiste Mathus]

Btw, I just remembered I already provide the xml export for my plugin: https://github.com/jenkinsci/buildtriggerbadge-plugin/blob/master/buildtriggerbadge-eclipse-formatter-profile.xml and ask contributors to try and follow it: https://github.com/jenkinsci/buildtriggerbadge-plugin#contributing though I neither enforce nor actively check formatting in PRs currently.

Cheers

[Manuel Jesús Recena Soto]

I have to say.....I totally agree with Kanstantsin.

We need a coding style guide. I like how Netty project is managing
this, using Checkstyle:
https://github.com/netty/netty-build/blob/master/pom.xml#L69

Regards,

> https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS7L3tJr8yfA_jYKu6gbvpJ7XAW2kOWAwB8ja_DO1OrOhg%40mail.gmail.com.

>
> For more options, visit https://groups.google.com/d/optout.



--

Manuel Recena Soto
* manuelrecena.com [/blog]
* linkedin.com/in/recena

[James Nord]

// @formatter:off

periodFormatter = new PeriodFormatterBuilder()
                .printZeroAlways()
                .appendDays().appendSuffix("d ")
                .appendHours().appendSuffix("h ")
                .appendMinutes().appendSuffix("m")
                .toFormatter();

// @formatter:on

Problem solved[1] - let's move on :-)

You can even configure checkstyle to pick up these comments to turn on/off checkstyle.

[1] at the expense of some crufy in the code that is pretty easy to ignore.

[Nigel Magnay]

You describe things with a very broad brush.  Earlier you extrapolated from a request to format code into a comment about management measuring the irrelevant.  Now you're declaring that the benefits my team of 10+ (using extreme programming as our methodology, pair programming everything, test driven development, etc.) found from automated code formatting over multiple years of working together should be ignored because "auto-code formatters are evil".  The sole specific example you offer is that calls to fluent API's aren't we'll preserved by automatic code formatters.

Can you offer other concrete examples of cases (preferably in the existing code) where an automatic formatter will have a serious negative impact?

​I still struggle to see the purported benefit.​ It feels like a collective delusion - that we've carried forward an idea from the PL/1 and FORTRAN era (where, for example, long lines are particularly bad because soft-wrapping editors don't exist) because it somehow 'feels right'. "Nicely formatted code is a benefit... because formatted code is nice". Like much in software engineering, we sleepwalk into things because it seems to be what other people are doing (appeal to authority) or out of fear that 'you can't be considered a professional unless you're doing X', or that we have no way of demonstrating Quality, so we substitute Consistency in its place and hope nobody notices.

The example I gave is hardly conjured-up. Fluent APIs are all over the place. Here's a docker-plugin example

String tag_image = client.commitCmd(containerId)

​                                                  ​

.withRepository(theRun.getParent().getDisplayName())

​                                                  ​

.withTag(theRun.getDisplayName().replace("#", "b"))

​                                                  ​

.withAuthor("Jenkins")

​                                                  ​

.exec();

​becomes

String tag_image = client.commitCmd(containerId).withRepository(theRun.getParent().getDisplayName())

​                                                        ​

.withTag(theRun.getDisplayName().replace("#", "b")).withAuthor("Jenkins").exec();

 

So I find the 'benefit' of a formatter - for example - caring about the number of spaces after an open-bracket vs the semantic destruction above to be Very Bad Tradeoff, so people end up adding horrific comment-cruft to coerce their formatter into doing 'the right thing' without ever pausing to challenge the actual claimed benefit in the first place.

When I look at the git-client-plugin source code, I find relatively few cases of the fluent calls being badly damaged by an automatic formatter.  There are many fluent calls, but most of them will be untouched by an automatic formatter.

There is a trade-off between the simplicity of automatically maintained code format (with occasional sub-optimal formatting as a result of the automation) and the work I must do on every pull request to not make code formatting consistency worse in the two plugins I maintain.

​That is only a tradeoff if you care about the format (being 'ultra-consistent'), and you're ignoring the fact that you're not avoiding the work, you're pushing it out onto other people (who of course must juggle all the 'specialist rules' for each individual project). ​I'm suggesting that's a red herring and that there's a big opportunity cost (which is why, for example, nobody is forming a credible business case for the (sonar estimate of 20 man years effort) to 'fix' the 100,000 'major and above' checkstyle issues in the JDK. 

By all means, have some recommendations, a 'house style'. Exportable IDE settings are a great idea. Review and adjust code where the formatting makes comprehension and maintenance actively easier (like - naming, logical grouping, dead code elimination, all that other good, hard stuff that needs your brain engaged).  Concentrate on substantive issues (think about the maintainer) rather than the trivial -- checkstyle mostly drowns you in irrelevance.

 I'd also far rather commit changes that modify/'correct' styling (and pay the diff-wall tax) from those that seemingly care deeply than erect yet another barrier to contributors.

And as someone else pointed out - it's far harder picking over all the @deprecated obsolescence in core than it is visually parsing the code. 

[Daniel Beck]

On 27.10.2015, at 11:17, Nigel Magnay <nigel....@gmail.com> wrote:

> The example I gave is hardly conjured-up. Fluent APIs are all over the place. Here's a docker-plugin example
>
> String tag_image = client.commitCmd(containerId)
> ​ ​.withRepository(theRun.getParent().getDisplayName())
> ​ ​.withTag(theRun.getDisplayName().replace("#", "b"))
> ​ ​.withAuthor("Jenkins")
> ​ ​.exec();
> ​becomes
>
> String tag_image = client.commitCmd(containerId).withRepository(theRun.getParent().getDisplayName())
> ​ ​.withTag(theRun.getDisplayName().replace("#", "b")).withAuthor("Jenkins").exec();
>

All of your examples so far are formatters unwrapping manually wrapped lines.

I have a difficult time imagining this is something that cannot trivially be disabled in an auto-formatter. We could easily make this a criterion for any code formatter we choose.

What other specific changes do you object to?

[Daniel Beck]

On 27.10.2015, at 11:17, Nigel Magnay <nigel....@gmail.com> wrote:

> And as someone else pointed out - it's far harder picking over all the @deprecated obsolescence in core than it is visually parsing the code.

Good news, everyone!

http://jenkins-ci.org/content/jenkins-20-proposal-introduce-policy-api-deprecation

(Let's continue that particular discussion in https://issues.jenkins-ci.org/browse/JENKINS-31035 )

[Kanstantsin Shautsou]

It not auto-formatted in my IDEA :) I don’t remember changing anything specific for this. Only small tune based on Stephen C. contribution.md with imports *.

Never was a problem. And there is no such point in initial proposal. And of course it obvious that formatter should change builders/stream/etc.

PS Nigel, please disable your HTML formatter, small blue font is hardly visible.

You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/8fjvXGYbFJ4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/286490ff-5ad8-49a5-af41-da2bf0a7ff3b%40googlegroups.com.

[Arnaud Héritier]

AHHH good points to remember me about imports orders and rules (when to use a wildcard ...)

Before each PR I have to loose several minutes to be sure that my IDE correctly inserted the imports in the right place because all projects aren't using the same configuration...

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/0B1AB425-AD96-404C-8857-83DD2D9837C8%40gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

-----

Arnaud Héritier

http://aheritier.net

Mail/GTalk: aheritier AT gmail DOT com

Twitter/Skype : aheritier

[Kanstantsin Shautsou]

One more example of bad documentation https://github.com/jenkinsci/jenkins/pull/1855#discussion_r41438672

"New comer" tries follow mentioned in documentation 80 symbols width coding style. 

Seems automation can't handle such cases, but having documented styling should exclude such misunderstanding.

[Kanstantsin Shautsou]

<sorry-for-offtopic>

http://techbeacon.com/zombie-code-when-maintainability-goes-out-window
</sorry-for-offtopic>

[Ullrich Hafner]

I would also recommend to define and publish an official coding style. This is practice for all professional software projects. I don’t see why this should not be done in an open source project. If we can’t agree to an overall style we should at least start with a small subset of conventions that can be extended step by step. (But this is just my opinion as a plug-in developer, it is a decision the core team should actually make.)

 

It was really hard to argue with students in my testing class why the acceptance test suite has no official coding standard while the students learn in all lectures that a project team should follow a coding standard. So we ended up in forking the project, creating and committing a coding standard for the class, and finally removed the standard after the end of the semester.

I’m using a committed coding standard for my plug-ins and it works quite well. Normally, PRs follow this coding standard (at least after mentioning it).  

I also think that we should first define a coding standard. *After* that we can discuss if it is automatically applied or just provided as formatter preferences.  I the latter case we should enforce these rules using static code analysis.

(Note: a coding style consists of formatter preferences as well as a set of coding practices or guidelines, e.g., fields must be private, etc.) 

Am 02.11.2015 um 14:54 schrieb Kanstantsin Shautsou <kanstan...@gmail.com>:

<sorry-for-offtopic>

http://techbeacon.com/zombie-code-when-maintainability-goes-out-window
</sorry-for-offtopic>

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/9063c95b-b8b2-42b9-87e5-a63fd0c8ce06%40googlegroups.com.

[Kanstantsin Shautsou]

@Ulrich, see my initial post, i proposed "conservative" start rules and proposed automated github PRs verification that doesn't block PR merge and only indicate with comments on possible bad places.

One more addition https://github.com/jenkinsci/jenkins/pull/1907#issuecomment-154416520

[Baptiste Mathus]

This Jenkins 2.0 epic about OOTB experience might be either enlarged or another one added for developers, and not just 'users'. 

And actually Kohsuke himself also (initially?) had in mind to have also "Candies for developers" in 2.0.

Sure, the thing is that this subject seems to be seen by many as actually not candy but something blocking. And some others (like me) think that having something is actually comforting and make onboarding new developers easier. Somehow like a CoC it can clarify things that generally can sound obvious, but sometimes prove to be not that much...

I think that we could settle on some basic conventions.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1595bf88-3802-4f7d-a691-e2d2b149947a%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[christophe...@gmail.com]

> My proposal is to chose some coding style ...

I thought the project already had a style guide; one has already been published for the project, anyway.  The following appears in the "contributing" wiki page, originally authored March 2011 with latest revision in August 2015.  This page can be reached by following links from the Jenkins homepage.

"In the core, we try to loosely follow the Oracle Java Code Conventions."[1]

Is this not (or no longer) accurate?  If not, this verbiage is misleading and should be changed.

[1] https://wiki.jenkins-ci.org/display/JENKINS/contributing

Christopher Simons

[Baptiste Mathus]

Well, yeah, but I guess the thing is:

> "We *try* to *loosely* follow"

Might have left a lot (too much?) of room for debate :).

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/d0a010d2-54c6-4b24-a1d5-76aa4692c3fe%40googlegroups.com.

[Kanstantsin Shautsou]

JFYI, latest checkstyle version (with latest maven-checkstyle) supports more flexible configurations like allow inline if’s without brackets. Different types of inline annotations (configuration 1.3 DTD).  http://checkstyle.sourceforge.net/releasenotes.html

You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/8fjvXGYbFJ4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS6jf9JYzrezqRLobmyz13qmyxUGm5XjxPrdD8zcxxC_CQ%40mail.gmail.com.

[Stephen Connolly]

On Tuesday 10 November 2015, Kanstantsin Shautsou <kanstan...@gmail.com> wrote:

JFYI, latest checkstyle version (with latest maven-checkstyle) supports more flexible configurations like allow inline if’s without brackets. 

Just because you can doesn't mean you should. I favour always using block delimiters for if and else

 

Different types of inline annotations (configuration 1.3 DTD).  http://checkstyle.sourceforge.net/releasenotes.html

None of those match "Jesse"-style so hopefully we can kill that abomination off ;-)

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/B95A8E77-94F7-4E9D-8253-08CF6971019B%40gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
Sent from my phone

[Jesse Glick]

On Tue, Nov 10, 2015 at 3:22 PM, Stephen Connolly
<stephen.al...@gmail.com> wrote:
> None of those match "Jesse"-style so hopefully we can kill that abomination off ;-)

CheckStyle is a syntactic, not semantic, processor so it cannot
distinguish between adjectival annotations, determiners, and type
qualifiers.

[Stephen Connolly]

Which is why we should put annotations on the line(s) before the method/field declaration

 

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr2QF7BgnazAaf6w2p_N2Fcu5bGoGgvSFbQUo9DSdsvpxQ%40mail.gmail.com.

[Raquel Pau Fernández]

Hi to everyone,

I have created an open source tool called Walkmod for this purpose (link - http://www.walkmod.com). In fact, I executed Walkmod into Jenkins (core module) to remove dead code. This PR contains the results https://github.com/jenkinsci/jenkins/pull/1957. 

Walkmod allows you define your code conventions and fixes the code. It is possible because code conventions are implemented as code transformations. After all code transformations are executed, Walkmod writes the file using  an specific Eclipse formatter or just modifying the original source file with the minimum semantic or syntactic changes (e.g add @Override).

Moreover, there is a service called walkmodhub whose purpose is to execute walkmod incrementally for each push. Then, walkmod sends a PR with the necessary changes and nobody needs to change their habits.

Let me know your opinions.

Raquel

[Kanstantsin Shautsou]

Sorry, but imho this PR kills code. Will be glad to hear why jenkins should add walkmod that i never saw in any projects instead of mass used checkstyle (this thread is about styling). Modified parts (Serializable, readResolve()) in PR reportable by FindBugs that is static analysing tool. 

You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/8fjvXGYbFJ4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/33067c26-6b0a-482c-9523-a723e0518adb%40googlegroups.com.

[Raquel Pau Fernández]

Thank you for your opinion, 

However, If you think is that this PR is not valid for the Serialization, I am creating a convention that automatically makes classes to implement Serializable if contains readResolve. In fact, the problem that walkmod address is autofixing issues that appear over and over again. External people, like me, can do the same error manually and, in my opinion, if conventions are explicitly defined, these kind of errors can be automatically fixed.

FindBugs reports issues, but does not fix them. The result of this PR is produced by Walkmod, not by me. Walkmod autofix bad practices because you are automatizing these kind of corrections and thus, saves time.

[Kanstantsin Shautsou]

I hope keep this thread as it named in topic and don’t touch static analysing.

PS. You can’t blindly trust to static analysing results, they are only helpers and should never be used in autofix mode. Example of Serialisation fix attempt https://github.com/jenkinsci/jenkins/pull/1945

 

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8326ef25-1a2e-47c2-b00e-3f7053a7a49f%40googlegroups.com.

[Raquel Pau Fernández]

1) Ok, no problem, but I think that code style is really related with static analysis tools, because these tool will be able to validate or fix your code style.

2) I understand that you need to validate exactly what are your conventions before setting them as part of your project. After that, your trust in the tool like you trust in code editors.

3)  I have readed the suggested PR you send me, but I can't completely understand yet. XStream, as far as I know, uses the readResolve when it is dealing with the ObjectOutputStream and according the official Java documentation, just Serialiable and Externalizable elements can use readResolve. So, I don't understand why those classes that contain readResolve do not implement Serializable. Anyway, these are the Jenkins conventions/code style :-) and it is what I think that should be defined in somewhere.

[Manuel Jesús Recena Soto]

(in line)

2015-12-21 11:22 GMT+01:00 Raquel Pau Fernández <raqu...@gmail.com>:
> 1) Ok, no problem, but I think that code style is really related with static
> analysis tools, because these tool will be able to validate or fix your code
> style.

+1000. We need rules and guidelines, and "something" to validate/check it.

It is not productive to discuss the same things with the same persons.

> https://groups.google.com/d/msgid/jenkinsci-dev/abf1c0d0-f627-42af-8409-335ff0888831%40googlegroups.com.

>
> For more options, visit https://groups.google.com/d/optout.



--

[Kanstantsin Shautsou]

On Dec 21, 2015, at 13:22, Raquel Pau Fernández <raqu...@gmail.com> wrote:

1) Ok, no problem, but I think that code style is really related with static analysis tools, because these tool will be able to validate or fix your code style.

Found, according to wiki, checkstyle is also static analysing. I will clarify then and say that this thread is only about ‘coding style’ - how code looks, see initial post. 

2) I understand that you need to validate exactly what are your conventions before setting them as part of your project. After that, your trust in the tool like you trust in code editors.

Not blindly trust, but follow.

3)  I have readed the suggested PR you send me, but I can't completely understand yet. XStream, as far as I know, uses the readResolve when it is dealing with the ObjectOutputStream and according the official Java documentation, just Serialiable and Externalizable elements can use readResolve. So, I don't understand why those classes that contain readResolve do not implement Serializable. Anyway, these are the Jenkins conventions/code style :-) and it is what I think that should be defined in somewhere.

Please start new topic as this is the question that everybody asks when starting coding jenkins. That not how code looks, that how it works. 

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/abf1c0d0-f627-42af-8409-335ff0888831%40googlegroups.com.

[Jesse Glick]

XStream does not require use of the `Serializable` marker interface.

[Tomas Bjerre]

Google has a brilliant tool for formatting java code:

  https://github.com/google/google-java-format

I use it in all my projects. There are Gradle and Maven plugins:

  https://github.com/jenkinsci/generic-webhook-trigger-plugin/blob/master/pom.xml#L148

If you want to force something to appear on a new line, just add shaddow comments:

  private static Gson GSON =

      new GsonBuilder() //

          .serializeNulls() //

          .create();

I would strongly encourage all other maintainers to use it. Makes it a lot easier to contribute to your projects when the contributor don't need to spend time fiddling with formatting.

Den måndag 26 oktober 2015 kl. 16:24:19 UTC+1 skrev Kanstantsin Shautsou:

Every time i open core code to fix something my eyes become red because of unreadable code. The same i heard many times as bad argument for jenkins on conferences.

This doesn't cause any backward compatibility issues and cherry-pick shouldn't be a problem for LTS because near 2.0 LTS i think will be handled in different way.

My proposal is to chose some coding style and follow it in future.

"Any code style" rule today led to hardly readable code:

- Mess of spaces vs tabs: bad diffs

- Annotation mess: when i'm reading code i expect access modifiers to be as keywords on start of line. Project may have different annotations and eyes can't jump to everybody known keywords.

- Line width: scrolling code extremely inconvenient (imho even 120 sometimes not enough, but producing >150 enforces scrolling). 

- Random spaces around if/for, looks like some developers coded from calculators. All IDEs can auto format code (i also lasy to type spaces in right places, but i usually auto-reformat before commits).

- {} braces for if/for bodies: enough to make bug one time to understand.

Many experienced developers already added rules in their plugins and imho for core something neutral can be chosen. Imho oracle coding style is the most used and can be adopted. For example Stephen C. already has documented variant that mostly matches Oracle code style.

- Code will be more attractive for newcomers.

- Will allow automate checks. 

WDYT?

[Liam Newman]

To everyone one on this thread who had a strong opinion about formatting:

This sounds like something that could and should be put into a Jenkins Enhancement Proposal.  This discussion petered out before but if there were a JEP in which to track the opinions and choices it might get more visibility/traction. The proposal could recommend the use of a formatter and provide agreed upon settings that all components and plugins should use. 

On the other hand, this would be a non-starter if there were strong opposition from the major contributors to jenkins core and a reasonable group of plugins.  What I see in this thread, is a lot of +1, +-0.5, etc.  I see a -0 from Jesse.  And then one person expressing a strong -1.   

Would someone (Tomas, Kanstantsin?)  be willing to pick this up and start at JEP for it?  

-Liam Newman