Change multiple value parameter recommendation

[runejuhl]

This changes the recommendation for how to handle multiple value parameters to the following:

If you can supply multiple values for an attribute it's common practice to
enforce the datatype as an array of values, even if the default is a single
item. This cuts down on code and remove some edge cases. An example for string
is Array[String[1]] instead of Variant[String[1],Array[String[1]]].

Note that previously the recommendation was to have a Variant type, but this
causes problems with values that contain Arrays, e.g. Variant[Tuple[String, Array], Array[Tuple[String, Array]]] (which would unintentionally flatten the
array inside the tuple).

I know this is a personal preference, and I wouldn't be surprised if I'm a small minority, but I'm open to any arguments for/against this.

[ekohl]

I agree with this recommendation. I never liked the x or list of x convention that appears to be common in Ruby. The code (either Puppet or its templates) also becomes easier when you only allow arrays.

I'm not so sure about the 80 column change. With markdown I'm also fine with long lines, but I'd say at least 100 characters is a lot more flexible, especially when you have URLs in there.

[alexjfisher]

@ekohl It's often to preserve backwards compatibility when adding data types to an existing module.

At the very least we need to warn about using flatten with more complicated examples.

[runejuhl]

I'm not so sure about the 80 column change. With markdown I'm also fine with long lines, but I'd say at least 100 characters is a lot more flexible, especially when you have URLs in there.

I'm fine with whatever, I just picked 80 chars because parts of the document was already wrapped to 80 chars.

[baurmatt]

Actually I like the existing recommendation as its easier for Hiera users, though I don't like that fact that a parameter can be two (or even more oO) data types.

module::parameter: foo

vs.

module::parameter:
  - foo

Regarding the column length change: I think 80 is way to short, 200 might be better. Personally I prefer to ignore this linting warning. However we decide, can we please do that change in a separate PR? It's basically impossible to read the current git diff.

[runejuhl]

@baurmatt that's why I made two commits 😉
Here's the diff for the meat of the PR: 7e4662d

[ekohl]

@ekohl It's often to preserve backwards compatibility when adding data types to an existing module.

That's a fair distinction.

[baurmatt]

@runejuhl Ups, missed that 🙈 Sorry!

[runejuhl]

@ekohl It's often to preserve backwards compatibility when adding data types to an existing module.

That's a fair distinction.

I wouldn't see this recommendation as changing that; that's still possible and likely the easiest way to add types to old modules. I still think the recommendation is worthy, though.

As long as these guidelines aren't strictly enforced anyway I don't see a problem in changing the recommendation, and I don't imagine the guidelines will be strictly enforced anytime soon... (which I personally wouldn't want anyway)

[bastelfreak]

@binford2k what do you think about this / is there an official recommendation?

[juniorsysadmin]

My preference is for one datatype, not a Variant, and making it a breaking change.

[binford2k]

I don't know that there's an actual recommendation one way or the other. I personally like the one or many ability, but I dislike having to do the casting myself. Since Puppet does not have a way to automatically cast the parameter, and since this is a recommendation, not a requirement then I'm ok with it.

I do suggest simplifying the wording that I noted though.

[binford2k]

imo, the complexity of this example takes away from its readability. I think I'd word it more like this and leave the rest as a thought exercise for the reader if they care to do so.

Note that previously the recommendation was to use a Variant type, but this added excessive complexity.

[runejuhl]

...forgot about this one. I've updated the PR with your text.