clarify bare words vs strings for include/contain/require

[bastelfreak]

Describe the Change You Would Like

This is a resubmission of https://github.com/puppetlabs/puppet-specifications/issues/160 and similar to https://github.com/puppetlabs/language-style-guide/pull/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.