I like the aligned arrows but it doesn't need to be a lint error.

The arrow alignment is one of the basic puppet style guidelines everyone on the net seems to agree on.

Since we want our puppet modules to be reused by others, we can't get rid of the arrow allignment, IMHO.

(I personally couldn't care less about alligned arrows, but still it's a convention that is globally accepted).

[while looking at the bikeshed] for me the main annoyance is doing the work twice, namely paying attention to arrow alignment while writing the code and fix it manually afterwards during code review when puppet-lint fails.

If we think arrow alignment (or other style guides) are important then IMO tools should do the work for us, namely we should puppet-lint --fix files before review (implementation details TBD, e.g. via [mr]ake)

I definitely appreciate aligned arrows when reading puppet code. I also use vim, so with the expection of nested arrows, where I 've more than once had to instruct puppet-lint to ignore arrow alignment, I 've had no problems with it. When reviewing, It does feel weird code reviewing a change where one parameter is added to a resource and yet almost the entire resource shows up in the diff due to arrow alignment changes. For me it's keep it and if we can get what @fgiunchedi is suggesting I will be thrilled.

Change 294282 had a related patch set uploaded (by Filippo Giunchedi):
[STRAWMAN] let puppet-lint fix arrow alignment

https://gerrit.wikimedia.org/r/294282

Personally, I'm not a fan of arrow alignment enforcement with automatic gating reviews like we do today.

It's common coding/style advice to always separate cleanup commits from functional commits. That is, when you go into some source file to make a small change, and then notice the whole file has style issues, you don't turn your 3-line fix into a 450-line commitdiff and set the commit message to "I fixed this small bug, and also reformatted the whole file because it was ugly". You do the cleanup either before or after the functional commit (often it's recommended to save those up and do them rarely, because too many of them makes a mess of history/bisection/blame, too).

Of course, you should only need cleanup-commits rarely if you're enforcing review of good style on incoming commits, which should be the norm! The arrow-alignment issue is at a strange intersection in this space, though. The real content changes in your diff can not-violate all other style guidelines, yet this one rule may require you to break the norm above and "fix formatting" outside the scope of your original code change.

In style terms, it looks nice and it's a little easier to read, so +1 for visual prettiness of the result.

However, it has nasty side-effects on the readability and reviewability of commit-diffs. Unlike other forms of whitespace enforcement (e.g. block/scope indentation), this one has consequences beyond the meaning of the core diff being committed. Adding a new key=>value to an existing list of 30 should not require whitespace diffs to the other 30, turning a 1-line change into a 31-line change. With a nested data structure it can get even trickier. When trying to comprehend a diff that "naturally" touches 7 lines in 5 files, I don't want to mentally wade through 87 other lines of indentation-bump noise.

I think if we keep arrow-alignment enforcement in full, the very least we could do to make it less-painful is ensure that all of our review-tools (e.g. gerrit and phab things) have a mode/checkbox/toggle/whatever to trim whitespace from diffs being viewed, similar to git's -w option to several of its subcommands. It's not a complete fix, but it helps. Digging through history still gets more-complicated by mixing functional and cleanup diff-traffic in one commit.

A possibly-better choice would be to make the enforcement rule more-flexible, where it selectively enforces arrow-alignment depending on the situation. A commit of a whole new arrow-block should enforce alignment. Adding a new key that's shorter than existing keys should also enforce alignment (as there's no "noise" in that, you just need proper spacing on the line being added). But adding one new longer key to set of 30 existing shorter ones shouldn't enforce a whitespace bump of the other 30 lines. I'm not even sure precisely how you'd codify rules that would make sense for the validation code. It also means new validated commits lead to a repo with outstanding validation issues when re-parsed as a whole, so it's not necessarily brilliant either...

Also: something's perhaps wrong with puppetlint when doing arrow-alignment-fix in isolation from other checks. In the strawman commitdiff for that, most of the changes are awful. I didn't see a single change that actually increased the readability of the result.

Aligned arrows is pretty common in puppet code out there. It is an old practice and lots of us are probably accustomed to it by now, so it's natural that at least some of us will be probably be averse to this kind of change :)

Since it is something we're fairly used to (and it's also widely spread across our tree) I'd personally like to see better arguments than "my editor doesn't support it" to be convinced. If the editor doesn't really support the official puppet style guide (or a feature as popular as this one), then maybe the style definitions for your editor are poorly written and you'll need to improve it if you want to continue using it :) Both vim and emacs do support it fine, I think.

The commitdiffs argument sounds a more appealing to me, however Gerrit already highlights whitespace (and is good at hightlighting differences within a line), so it's relatively easy to review such changes (I haven't personally had a problem reviewing code like that). For git diff/blame I use -w, for this an other whitespace changes that happen often anyway.

As for restyling an entire manifest in the same commit as making a non-stylistic change, that seems broader than just aligned errors and feels orthogonal to this discussion. We should definitely *not* do that and use separate commits for such purely stylistic changes. I can't even remember the last time I saw a "combined" commit pass by but I'll definitely call it out if I see such a commit. I assume we can all agree to at least that :)

Like @BBlack, my main issue with alignment is the way puppet-lint forces me to re-indent a whole section just to add or remove one line.

For one thing, I would prefer if it let you over-indent, leaving a few columns of extra indention so that when you add a new row that has an identifier slightly longer than all the others, you aren't forced to re-indent everything. And in the opposite situation, when removing the longest identifier it should not force you to reduce the indention of all the others. As long as they are aligned, the linter should not care how much whitespace is between the identifiers and the =>

In T137763#2380627, @mmodell wrote:

Like @BBlack, my main issue with alignment is the way puppet-lint forces me to re-indent a whole section just to add or remove one line.

For one thing, I would prefer if it let you over-indent, leaving a few columns of extra indention so that when you add a new row that has an identifier slightly longer than all the others, you aren't forced to re-indent everything. And in the opposite situation, when removing the longest identifier it should not force you to reduce the indention of all the others. As long as they are aligned, the linter should not care how much whitespace is between the identifiers and the =>

On this one ^ I concur. It's my only problem with arrow alignment. Older (way older) versions of puppet-lint in fact did not have that check and would accept any amount of whitespace between the parameters and the =>, but not so old ones do.

Please keep this check. I have fixed ALL of these across the entire repo before letting it vote. That was a lot of work and it has already been done.

It's easier on my eyes; that's one of the things style checks do is make it easier for the eyes to pass over code so the reader can concentrate on the substance of what is being read.

Yes, the whine about too much space (over-indentation) is annoying, I would love to see a fix-up for that in the future.

Having our review tools be able to ignore the arrow alignment and/or other space changes when showing diffs would be good too.

Change 294282 abandoned by Filippo Giunchedi:
[STRAWMAN] let puppet-lint fix arrow alignment

Reason:
T137763 has been declined, can be revisited later, e.g. with different puppet-lint versions

https://gerrit.wikimedia.org/r/294282