style guide: Stop aligning parameters at = and/or $

[bastelfreak]

I think the 'old' style of aligning parameter is quite bad and we should stop doing this.

[ekohl]

👍 for this, though I will note that we shouldn't make it mandatory to refactor a whole module for this when a single parameter is added. Consistency within the file is probably more important.

[bastelfreak]

Adding a new paramter should not cause a refactor. But I think we should enforce the new style for new parameters and ignore the existing style in a file. What do you think about that?

[jstraw]

If we are changing this for a class/define variable list, are we planning to change this for a resource definition's => which has the same "feature." I agree with the change, but I might agree less with the resource definition change (because the => alignment makes it easier to read)

[bastelfreak]

this is only about the parameter list in classes/defined resources. not about resource declaration. We've a puppet-lint plugin that enforces the alignment there and I think we should keep it. The diff for resources is usually way way smaller.

[jms1voalte]

Personally, I find it easier to read code when things are all vertically aligned. Also git diff does have a -w option to not show lines where the only change is whitespace, and some git web interfaces have a checkbox to do the same thing. (For me this is true of almost every language, not just Puppet/Ruby.)

As long as there's a way to tell the linter to accept the old format (or to ignore whitespace between tokens entirely), I have no objections.

[spotter-puppet]

There's a slight typo... "Sometimes were was an additional alignment". I believe this should be "Sometimes there was an additional alignment".

[zilchms]

I would agree that the downside of having to refactor when adding a parameter is not good. However I am not so sure about the longterm readability.
For long parameter lists I think the old style makes it a lot easier to search for a specific one (when ordered alphanummerically).
On short parameter lists the old style is very cumbersome and makes them less readable.
I think currently we dont order alphanummerically first, instead we order by required parameters and optional parameters (I think)?
In that case the old one doesnt really add much value in my opinion, so we could drop it.

[smortex]

Just like other people here, my preference go to aligned types / names / default value because it is ways more comfortable for me to work with these lines when items are vertically aligned.

For sure, rebasing commits that change types can be a PITA, but only when we change the column where alignment take place. When using a syntax that use tabs, it's easy to use say "2 tabs" in general and if a line overflow, just put a single space and not align this line; but because puppet files don't use tabs (and 2 spaces soft-tabs), when I have a little overflow I trend to "fix" it by re-aligning all members… leading to these conflicts. But I am ready to pay this rebase conflict risk when dealing with such changes 🤷.

Having a rule that says that we should align fields is probably an annoyance. Maybe we can just remove this line? If people have to deal with aligned fields and it's annoying, they can remove the extra spacing and be happy. If people have to deal with unaligned fields and it's annoying, they can add extra spacing and be happy. I don't think that happen so often that we will have to deal with issues where a module go back and forth from a form to the other; and since people trend to contribute to some modules and not all, the module they contribute to the most are likely to use the alignment that fit them best in the end?

[davidsandilands]

I would still bias to the alignment of = making it easier to read for me but I feel the frustration it creates when a new parameter knocks everything about.

[bastelfreak]

I would still bias to the alignment of = making it easier to read

I understand that. But should the parameters be readable? That's the whole purpose of the REFERENCE.md. Provide the readable list of all parameters, their datatypes and the default values.

[rwaffen]

I find it easier to read if I have some space between things, for syntax and code for itself it is not required i know, but to get an overview and read and compare things I very much like this alignment. It is not nice to rebase, but I think it is worth the work to keep things aligned to better understand/read/view them.

[tuxmea]

My preference:

1. Ordering

First we have paramaters without defaults, odered alphabetical.
Next we have all parameters with defaults, ordered alphabetical.
If the module has more than one entry point (e.g. server and client) the parameters should be grouped and then ordered alphabetical.

2. Alignment

All parameter groups (see above) should align at $ and =

[bugfood]

I do agree that alignment increases readability, but personally, I don't think it's worthwhile, given the documented annoyances for modification.

I stopped aligning parameter sections shortly after I started declaring the data types, since the lines were getting way too long.

The use of should indicates to me that this is a recommendation, not a requirement, so this doesn't seem like such a big deal either way.

[ekohl]

Couldn't agree more with what @bugfood wrote. Data types made alignment impractical. For complex data types (or just long names) it breaks readability by making the lines so long that you can no longer easily see what the lines are. So you may mistake the data type from the line below with the default value from the line you're reading.

[rwaffen]

if data-types get to long, it breaks everything, right. but this is a sign for me to shorten this thing again by writing my own data-type. not really related, but a thing i do often when i stumble upon long data-types 😄

[ekohl]

Also a fair point about custom data types, though if you introduce a custom data type and only use it once that's not a good experience for the module consumers. Perhaps a separate discussion.

I think @bugfood said it best: we have the word should. So that gives flexibility to who's making changes. What is important to me is that we're consistent. I guess there are 3 levels we can be consistent (and not at all):

• organization (our entire GH namespace)
• module
• file
• not at all

My suggestion is to keep it consistent at the file level. I love consistency, but I usually don't care beyond a single file.

[jstraw]

It looks like lazy consensus suggests either leaving this as is or editing it to stay as a "keep consistent at the X level" rather then a hard and fast rule. If there's no updates in the next week I'll close this to leave it as is.

[yakatz]

The type declaration for a parameter can potentially add a huge amount of white space. I'm in favor of strongly encouraging type specifications and not requiring alignments.

[saz]

I stopped aligning in my code base, especially as I'm making use of renovate comments to keep versions up to date and aligning makes it more annoying for me (e.g. accidental misalignment of such a comment)

[jms1voalte]

FWIW ... if I have a list of parameters with some short and some long, I'll put the parameter names with long types on the next line, vertically aligned with the "normal" parameters in the same group. Example:

define dummy::service (
      String[1]           $svc_dir      = "/opt/${title}" ,
      Pattern[/^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/]
                          $listen_addr  = '127.0.0.1' ,
      Integer[1024,65535] $listen_port  = 12345 ,
      Array[Pattern[/^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/]]
                          $allow_from   = [] ,
    ) {

  # declare all the things
}

I'm not sure how this aligns with the style guide, but after 13 years of writing Puppet code, this is the convention I've settled on for handling parameters with "long" data types. The compiler doesn't seem to have any problems with it, my old eyeballs find this easier to read, and I don't have to horizontally scroll the window.

(And before anybody asks, yes, I do have a custom data type for IPv4 addresses, which only allows valid addresses. This is just a quick contrived example to illustrate how things are vertically aligned in most of my code.)