clarify bare words vs strings for include/contain/require

[bastelfreak]

Describe the Change You Would Like

This is a resubmission of puppetlabs/puppet-specifications#160 and similar to #18

Use Case

include/contain/require are basically function calls. They are usually used like this:

include foo
require baz::bar

or:

include 'foo'
require 'baz::bar'

I think it's a bit confusing that you can quote it but you don't have to. This makes it confusing for new users because in the first example you've a string and don't need to quote it, but in other code places you have to quote strings. I think we should settle on one style and enforce it with a puppet-lint plugin

Describe the Solution You Would Like

Always quote arguments for include/require/contain etc.

[davidsandilands]

I would be against enforcing as this would create a lot of change on users, from experience with customers and users and our own documentation https://www.puppet.com/docs/puppet/8/lang_classes.html#include-function we never use quotes.

[bastelfreak]

our own documentation https://www.puppet.com/docs/puppet/8/lang_classes.html#include-function we never use quotes

Yes. I want to point out that this repo is the style guide, not https://github.com/puppetlabs/puppet-specifications/. I want that the Puppet DSL still accepts bare words but the style guide and puppet-lint should recommend quotes. Quotes are even used in the pe_install module within PE.

[davidsandilands]

Sure but I guess what I'm saying is with a reasonable expectation a sizeable number of people will have done it without quotes updating the linter to recommend quotes by default will create a lot of noise.

[alexjfisher]

FWIW I prefer without quotes... As long as we're not writing any of the following, I'm happy!

'foo'.include()
foo.include
include([
  'foo'
])

All of those are valid and do the same thing as include foo. This type of function is called a 'statement keyword' in the code. They are defined here. https://github.com/puppetlabs/puppet/blob/82ad86ea21ecf20a956db6bf46140940caccd0cc/lib/puppet/pops/model/factory.rb#L970-L995

[alexjfisher]

I'm submitting

$class = foo
[
  $class,
].include(
)

as the 'most cursed without triggering existing puppet-lint plugins'. 😆

[bastelfreak]

@alexjfisher I think it's fine that this is working, but I think puppet-lint should warn about it.

[bastelfreak]

I had to debug an outage for a larger PE customer today. They basically did something like this:

$foo = bar
include foo

But what they wanted was:

$foo = bar
include $foo

since bare words are valid, this passes basic code validation and requires unit-tests to detect that a class named bar doesn't exist. Having quotes would help and enable us to detect missing $ via puppet-lint.

[rwaffen]

I want to extent the above example.

given the following code:

$data = {
  'something' => 'value'
}

$var = data['something']

data is missing the $. It should be $data instead of data.
But bare words are allowed in Puppet. So the code is valid.
But it shouln't be valid. It should be an error.

It should be $var = $data['something'].
Or it should be $var = "data['something']".
Everything else should be an error.

[jhoblitt]

It should be $var = $data['something'].

I agree that the sigil should be required in all cases.

[jhoblitt]

My weak personal preference is that class names, E.g. require foo, are not quoted. I feel strongly that the allowed syntax, whatever it is, is consistent and unambiguous.

[hlindberg]

One thing that bare words give you is that they are syntax-checked. Compare:

include "foo:bar" # note single colon
include foo:bar

The first would not be caught until evaluated (i.e. compile time) since the syntax is valid, but the name is wrong.
the second produces a syntax error.

[bastelfreak]

both of those examples can easily be spot by puppet-lint and it shouldn't be an issue for Puppet itself to catch it.

[hlindberg]

How hard is it to understand that strings are written with single or double quotes and that for certain well-formed strings that function as names/enums, you do not have to quote them?

[hlindberg]

@bastelfreak surely you agree that the more that puppet itself catches actual errors the better...

[ekohl]

I think @hlindberg's arguments are good and it's also such a well established way that I lean to:

Arguments to include SHOULD be bare strings, variables or arrays but MAY be quoted strings.

Recommended:

include foo
include foo::bar
include $var
include $var['key']
include [foo, foo::bar]

Allowed:

include "foo"
include "foo::bar"

given the following code:

$data = {
  'something' => 'value'
}

$var = data['something']

Is this even valid syntax? I think the syntax checker should catch data['something'] already but otherwise a lint plugin can probably catch it.

[kjetilho]

Is this even valid syntax? I think the syntax checker should catch data['something'] already but otherwise a lint plugin can probably catch it.

Yes, it is valid syntax, but it will always raise an error.

data["foo"]

Error: Evaluation Error: A substring operation does not accept a String as a character index. Expected an Integer (line: 1, column: 1)

however: data["2"] yields "t" (the third character of the string "data").

I agree with @hlindberg that we should continue to allow barewords for class names in include/contain/require. I'm not so happy with barewords elsewhere, like in the example above. (I got to this issue after I wrote facts['os'] in my own code.)

In my own code, I only use barewords for ensure (historical style guide I think?) and class names. I would love a lint warning for barewords outside these contexts.

[ekohl]

To me bare words also feels more consistent with a define:

foo::bar { 'title':
}

On the other hand, with class I typically use strings again because I always do that with titles.

class { 'foo::bar':
}