Announce: Puppet Language Style Guide 2.0

[Lauren Rother]

The Modules team and I are excited to finally announce the newest version of the Puppet Language Style Guide.

We've reworked the guide to reflect the new features and capabilities of Puppet 3.7, and we've expanded it to cover more topics related to building manifests and modules. If you're interested in publishing a module to the Puppet Forge or are looking to get your module "Puppet Approved," the updated guide is a great place to start.

It was a massive, company-wide effort to update this style guide, but I'm sure we didn't catch everything. If you notice a mistake or would like more information on something that's not currently covered, please file a ticket. We plan to regularly update the guide from here on out, and we definitely anticipate another big release in the months after Puppet 4 comes out.

Puppet Language Style Guide:
https://docs.puppetlabs.com/guides/style_guide.html

File a ticket:
https://tickets.puppetlabs.com/browse/DOCUMENT/

The Puppet Approved program:
https://forge.puppetlabs.com/approved

Thanks!
Lauren

--

Lauren Rother

Technical Writer

Puppet Labs, Inc.

[Garrett Honeycutt]

> *Lauren Rother*

> Technical Writer
> Puppet Labs, Inc.

Hi,

Awesome work on the new style guide! I've got a few questions and some
suggestions.

== Section 5
Why the arbitrary line length? Puppet's DSL does not have the ability to
break lines, such as '\' in bash, so imposing any limit does not buy
anything, certainly not readability. If you want to give clear error
messages that improve readability they will include the variable and its
data which could be greater than 140 characters. In essence, until there
is a way to break lines in the DSL, there should not be any limit to how
long a line could be.

== Section 9.6
This suggests that using symbolic modes (mode => 'u=rw,g=r,o=r') are
acceptable. What is the purpose of this? This decreases readability and
understanding.

== Section 10.2
Would add that includes should happen right after defining parameters
and before validation.

== Section 10.4
Suggest adding that chaining arrow syntax should only be used with the
reference syntax and not between resources. The example below is a bad
use of the chaining arrows that leads to changing ordering by
inadvertently moving resources around. This is easy to do when merging
software.

package { 'foo':
ensure => present,
} ->

service { 'food':
ensure => running,
}

== Section 10.6
Suggest that while having required parameters for defines is OK, having
them for classes is not. There should never be required parameters for a
class. This breaks the ability to `include` a class.


== Section 18
This wording is not very clear. Are you saying that parameters should be
listed in the order that they are used instead of alphabetical order?
Agree with adding resources, though how this actually works with adding
parameters is unclear.


Best regards,
-g

--
Garrett Honeycutt
@learnpuppet
Puppet Training with LearnPuppet.com
Mobile: +1.206.414.8658

[Trevor Vaughan]

Inline....

On Mon, Feb 2, 2015 at 3:24 PM, Garrett Honeycutt <g...@garretthoneycutt.com> wrote:

<snip/>

Awesome work on the new style guide!

Completely agree!
 

== Section 5
Why the arbitrary line length? Puppet's DSL does not have the ability to
break lines, such as '\' in bash, so imposing any limit does not buy
anything, certainly not readability. If you want to give clear error
messages that improve readability they will include the variable and its
data which could be greater than 140 characters. In essence, until there
is a way to break lines in the DSL, there should not be any limit to how
long a line could be.

100% agree with this. I always turn off line length checking because otherwise, it's completely unreadable over time.

 

== Section 9.6
This suggests that using symbolic modes (mode => 'u=rw,g=r,o=r') are
acceptable. What is the purpose of this? This decreases readability and
understanding.

How do you do this with numeric modes?

mode => 'o-rwx'

I.e. I don't care what U and G are but O better not be rwx.

 

I would say that numeric should be 'preferred' but that symbolic should be completely acceptable.

== Section 10.2
Would add that includes should happen right after defining parameters
and before validation.

I'd like to vote for putting all validation at the bottom of the file.

Honestly, we're getting quite heavy in the amount of cruft in the files in general.

For a file with 30 parameters:

~ 120 lines of comments

~ 30 lines of parameters

~ 30 lines of validation

This is *before* you get to any code.  And, of course, the processor has to ignore all that stuff, which could be upwards of a couple of MB of data for large code sets.

Would moving the comments to another file be better? modulename/doc/init.md, modulename/doc/subclass.md, etc...?

Anyway, back to my point. Having 30 more lines of stuff to wade through before I get to code is just irritating, particularly when Puppet prints out the exact line for validation when it errors out causing an immediate lookup capability.

Should there be some standard for 'Here There Be Code?'.

For Example:

# Commenty goodness
#

class foo (

  ... 30 params that point to another file ...
) {

... 40 lines of validation magic ....

## I LOVE CODE ##

user { 'something_useful': } ...

== Section 10.2.7

I tend to order things in resource alphabetical order because then, if I'm looking for a file resource, it's in the 'F' section. And I still like the fact that order doesn't matter in Puppet unless I tell it to. Accordingly, should I happen to break my order accidentally, I really don't want to care.

== Section 10.4
Suggest adding that chaining arrow syntax should only be used with the
reference syntax and not between resources. The example below is a bad
use of the chaining arrows that leads to changing ordering by
inadvertently moving resources around. This is easy to do when merging
software.

    package { 'foo':
      ensure => present,
    } ->

    service { 'food':
      ensure => running,
    }

100% Agree. This is very difficult to troubleshoot.

 

== Section 10.6
Suggest that while having required parameters for defines is OK, having
them for classes is not. There should never be required parameters for a
class. This breaks the ability to `include` a class.

No, I disagree here. There are (many) times that I *need* you to give me a parameter, I can't make one up that is magically correct. Moving this to a define-only state means that we have to start slinging around singleton defines which is what parameterized classes got us away from.

I'd rather have my compile break than end up with a system doing something nonsensical, particularly when the security of the system may be at risk.

 

<snip/>

--

Trevor Vaughan
Vice President, Onyx Point, Inc
(410) 541-6699
tvau...@onyxpoint.com

-- This account not approved for unencrypted proprietary information --

[Trevor Vaughan]

Sorry for the double post but, in terms of validation, it would certainly have to come prior to other functions running so it probably does need to come first.

I'll stand by my comment of needing some consistent method for determining where the interesting parts actually happen as a community.

Thanks,

Trevor

[jcbollinger]

On Monday, February 2, 2015 at 2:24:42 PM UTC-6, Garrett Honeycutt wrote:
 

== Section 5
Why the arbitrary line length? Puppet's DSL does not have the ability to
break lines, such as '\' in bash, so imposing any limit does not buy

anything, certainly not readability. [...]

Agreed.

 

== Section 9.6
This suggests that using symbolic modes (mode => 'u=rw,g=r,o=r') are
acceptable. What is the purpose of this? This decreases readability and
understanding.

Agreed.

 

== Section 10.2
Would add that includes should happen right after defining parameters
and before validation.

Absolutely.

 

== Section 10.4
Suggest adding that chaining arrow syntax should only be used with the
reference syntax and not between resources. The example below is a bad
use of the chaining arrows that leads to changing ordering by
inadvertently moving resources around. This is easy to do when merging
software.

Excellent.

John

[jcbollinger]

On Monday, February 2, 2015 at 2:58:51 PM UTC-6, Trevor Vaughan wrote:

Inline....

On Mon, Feb 2, 2015 at 3:24 PM, Garrett Honeycutt <g...@garretthoneycutt.com> wrote:

 

I'd like to vote for putting all validation at the bottom of the file.

Honestly, we're getting quite heavy in the amount of cruft in the files in general.

Agreed.

 

== Section 10.2.7

 

I tend to order things in resource alphabetical order because then, if I'm looking for a file resource, it's in the 'F' section. And I still like the fact that order doesn't matter in Puppet unless I tell it to. Accordingly, should I happen to break my order accidentally, I really don't want to care.

Although I do tend to order my resource declarations as described by 10.2.7, I agree that it is not a particularly useful as a style guideline.  I use that order because it makes sense to me and helps me find things, not because of the functional implications (that anyway don't always apply), and I don't stick to that order rigorously.  I'd argue that a mandate to order resource declarations specifically by relative order of application is counter-productive.  It de-emphasizes the importance of using relationships to specify application order, and may sometimes mask bugs arising from failure to declare needed relationships.

 

 

== Section 10.6
Suggest that while having required parameters for defines is OK, having
them for classes is not. There should never be required parameters for a
class. This breaks the ability to `include` a class.

No, I disagree here. There are (many) times that I *need* you to give me a parameter, I can't make one up that is magically correct. Moving this to a define-only state means that we have to start slinging around singleton defines which is what parameterized classes got us away from.

I'd rather have my compile break than end up with a system doing something nonsensical, particularly when the security of the system may be at risk.

I think the middle ground here would be best: the guide should recommend minimizing the number of mandatory class parameters.  Although I very much favor declaring classes via 'include', I acknowledge Trevor's point, and I observe that mandatory class parameters can be satisfied by automated data binding (i.e. Hiera), so they don't altogether break the ability to 'include' such a class, they just limit it.


John

[jcbollinger]

On Monday, February 2, 2015 at 12:21:40 PM UTC-6, Lauren R wrote:

The Modules team and I are excited to finally announce the newest version of the Puppet Language Style Guide.

We've reworked the guide to reflect the new features and capabilities of Puppet 3.7, and we've expanded it to cover more topics related to building manifests and modules. If you're interested in publishing a module to the Puppet Forge or are looking to get your module "Puppet Approved," the updated guide is a great place to start.

It was a massive, company-wide effort to update this style guide, but I'm sure we didn't catch everything. If you notice a mistake or would like more information on something that's not currently covered, please file a ticket. We plan to regularly update the guide from here on out, and we definitely anticipate another big release in the months after Puppet 4 comes out.

I'm pleased to see PL directing time and attention to such an effort, and I congratulate you on bringing it to this milestone.

With that said, I'm afraid I have a quite a few criticisms of the current version of the document.  I hope you will find them constructive in nature:

Overall document issues

The very first thing that hit me when I sat down to read the guide was section 1, with its reference to RFC 2119 terminology.  What in the world is such a thing doing in a *style* *guide*?  That describes the terminology of a specification document.  If a specification is what you mean this to be, then kindly position it that way.

In fact, the terminology matter is symptomatic of a deeper issue: the document appears to be at least two different documents mashed together.  At times it wants to be a DSL style guide / specification, but at other times it wants to be a module layout specification (not much "guiding" on that side).  There is some overlap between those areas, to be sure, but it would be better to have two documents, each more tightly focused.  Some of my per-section comments will also reflect on this issue.

Specific issues with guidelines

Section 4: versioning is not a style issue.

Section 5: how is trailing whitespace a style issue of any consequence?  It has no bona fide relationship to the guiding principles enumerated in section 3 of the document.  It's not that I'm a big fan of trailing whitespace, but this seems so superfluous.  The only practical reason I see to include it (as a "must", no less) is to simplify the implementation of style checkers.  Not welcome.

Section 7: as a matter of style, why shouldn't comments explain the "how" of the code?  In the event that it is unclear how the code accomplishes the "why", documentation of that "how" is important for maintaining the code.  I suspect that there are particular practices that this guideline is trying to rule out, but if so then a more nuanced guideline is needed.  Additionally, I don't like either of the examples.  For what it's worth, the comment, if any, with which I would adorn the example declaration would be something like "Manages the main NTP configuration file".  (And I note that I would characterize that more as a "what" than as either a "why" or a "how".)

Section 8. Having module metadata is not a style issue.  I thought perhaps this point was about the style to be used for a module metadata file, which might be ok, but if that was the intention then the actual section is a complete miss.  Moreover, it is not clear either in the style guide or in the document it references what the metadata "format" actually is, unless the answer is simply "JSON".  Perhaps "format" is the wrong word for what you're trying to describe.

Section 10.2, item 7: in addition to comments in previous posts regarding this declaration-ordering recommendation, I observe that it conflicts with guideline 9.4 about putting declarations in logical groupings.

Section 10.2: the justification for the recommendation that "the last two items – declared resources and declared relationships to other classes/defines – not occur in the same class or type" is unclear.  I'm inclined to call such a recommendation altogether unjustified, but perhaps I don't understand what the guide is trying to say.

Section 10.7: the specified style for parameter defaults is fine for classes, but it is not applicable to defined types.  I don't know how to safely satisfy the guide's recommendation that "Provided defaults should be specified with the parameter and not inside the [...] define."

Section 11.2: WHAT?  In the first place, what "non-deterministic scoping issues" are we talking about here?  In the second place, how are classes supposed to declare other classes?  The only other alternative -- resource-style class declarations -- are inferior in most situations, as even the language reference acknowledges: "Most users in most situations should use include-like declarations" (emphasis in the original).  Is this issue even really about class declaration style at all?  If not, then why does it call out the "include" function?

Section 12.1: This is worded confusingly.  I guess it's trying to say that declarations of resources of a defined type should specify a resource title that includes a characteristic variable's value, so as to ensure that multiple instances' titles do not collide.  Although the text is unclear, I take issue even more with the guideline itself.  It is a design / implementation consideration, not a style issue, and more importantly, it is not valid as a general rule.  There absolutely are cases where I want to declare a defined type instance with a static title, just like there are cases where I want to declare an instance of a plugin type with a static title.  There is nothing inherently wrong with that.

Sections 16 & 17: These are not points of style, they are points of module organization / documentation / layout.  They do not belong in a style guide.

Section 18: What does this have to do with style?  Or even with module design / layout / etc.?  I don't object to it is a good general rule, but it does not belong in a style guide, and maybe not even in the other document(s) that is currently trying to live inside the same text.

Section 19: if the guide is intended to serve as a design document for the *-lint programs, then that would be better described in section 2 ("Purpose"), in those terms. If it really just "helps" development of those tools, then there is no reason to mention that in the guide itself at all.  If the point is supposed to be that those programs can help check compliance with the style guide then I think that's out of place inside the guide itself.  If in that case you insist on referencing the *-lints anyway, then you should put them in that context (i.e. compliance-management tools).


Issues with guide text

Section 3, item 2: the text of this item is nearly a complete miss for me.  The text doesn't really describe what it means by "simplicity", and it seems not to touch on "scoping" at all.  Even though I think I know what you're talking about, the text is pretty unclear with respect to the odd significance it attributes to the word "and".  Moreover, although some aspects of "scope" and "simplicity" might well be considered matters of style, what you seem actually to be talking about is not that at all; rather, it is a module design principle.

Section 6: the guide is inconsistent and incomplete with respect to quoting.  It says "all strings must be enclosed in single quotes" with certain exceptions, but it does not say what should be done in the exceptional cases (they should be enclosed in double quotes, presumably).  And then the very next item says certain other strings don't need to be quoted, in contradiction of the previous point.  The apparent intention there is just fine, but the text could use some work.

Section 10.1 seems to preclude manifests residing in subdirectories of a module's 'manifests' directory, as in fact is a relatively common practice.  I'm accounting this a text issue because I don't think it is actually the intention to exclude such arrangements (where they in fact correspond correctly to class / define names).

Section 10.5: it's unclear what "close to node scope" means in this context, if it in fact adds anything at all to what is already stated in the preceding sentence.

Section 10.5: The sentence "If you have a class or define which requires another class or define, graceful failures must be in place if those required classes or defines are not declared elsewhere" seems not to apply to the situations described in the examples.  This revolves around the meaning of the word "declare" (which perhaps should be defined in the document): both the examples revolve around (nested) definitions -- one of a class, the other of a defined type -- not around declarations.

Section 10.8: This whole section seems kind of murky to me.  I think it must be inspired by some specific cases, and perhaps I would understand if I were familiar with those cases.  As it is, though, the attempt at generalization is not working for me.  I understand the words, but I'm genuinely having trouble understanding the big picture of what the section is really driving at.

Section 11.1: Tucked at the end of this section is "Remember: Class inheritance should only be used for myclass::params parameter defaults."  That's a lot more specific than the rather large volume of text and examples preceding it.  If it's really what the section is trying to say then you should make it the guideline and lead off with it; otherwise you should omit it.

----

That's it for now.


John

[Hunter Haugen]

tl;dr Summarizing the feedback from Garrett, Trevor, and John (because email is hard), and see if we have resolution on some points and which ones we still need to clarify.

= Questions:

These need further discussion and clarification.

== Section 10.7 Defines can't use inherits for parameter defaults

Should this be reduced to only cover classes, or should the description be expanded to cover the style of setting defined resource defaults also? (Which happen to be the "bad" example; whoops.)

== Section 11.2 Recommendation against using "include" is unclear or harmful

I think this was to describe how an over-zealous use of `include` may cause classes to be declared with the default parameters before a user may declare them with parameters. Or it could have been about cross-module class inclusion in public modules. What should we do with this section?

= Proposals:

If the below changes sound good, then we'll go with that.

== Section 5 line length is an issue

Managing line lengths in puppet is difficult. This point should change to describe where to do syntax-wise linebreak rather than character-wise breaks (ie, any hashes containing more than a single key/value pair, etc.).

== Section 9.6 symbolic modes

This was answered in that symbolic modes can perform operations that numeric modes cannot. The style guide does not dictate which form to use, so this should not be an issue.

== Section 10.6 No required class parameters

Some classes just MUST have required parameters and there is no way around it. The style guide should be modified to include "should minimize the number of required parameters" as a style recommendation.

== Section 10.2 item 7, resource ordering alphabetically

Remove the mention of ordering so that it is simply "Should declare resources." This also impacts section 9.4 and should be reconciled.

== Section 10.2 includes vs validation first

Validation cannot come at the end of the file due to parse order, but it is possible to allow includes to come before validation, so we should just not dictate this bit of ordering.

(This issue will go away entirely after the release of puppet 4, as validation may happen inline with the parameters themselves due to the typing system.)

== Section 10.2 Unclear description of "items 7 & 8"

I'm not exactly sure what this is describing either. Most likely superfluous and will be removed.

== Section 10.4 Chaining arrow syntax with only references, not declarations

Agreed. And perhaps that if there are line breaks around arrow syntax, they must only happen to the right of the arrow and not the left so that arrows are not "hidden" at the end of previous lines.

== Section 12.1 $unique_name = $name is unclear

I believe this was to describe how the continued use of $name throughout a define can lead to confusion, as $name has no strong semantic meaning. Thus a "good" example would be https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/listen.pp#L2 and a bad example would be... https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/vhost.pp (because $name is scattered throughout the define and has no definite meaning).

== Section 18 Parameter ordering unclear

The paragraph about ordering should be moved to 10.6 or removed altogether.

== Section 19 should be moved to Section 2

Because if becoming a specification for puppet-lint is a purpose, then section 2 is best.

== "Style guide" vs "specification" discussion from jcbollinger

To paraphrase, "the 'language style guide' is written as a technical specification covering more than the language; it covers module structure and module contribution conduct. This is not 'style' and is beyond the scope of this document."

In answer... we should change the title to something other than "The Puppet Language Style Guide" :). I personally feel that "Module Style Guide" would still encompass what is included here, when the above edits are taken into account.

== JCB's "issues with guide text"

These seem fairly straightforward. Thanks.

-Hunter

--
You received this message because you are subscribed to the Google Groups "Puppet Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-users/011324f7-4983-4baf-891d-e8b8a72f7a8c%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[jcbollinger]

On Tuesday, February 3, 2015 at 4:00:19 PM UTC-6, Hunter Haugen wrote:

tl;dr Summarizing the feedback from Garrett, Trevor, and John (because email is hard), and see if we have resolution on some points and which ones we still need to clarify.

= Questions:

These need further discussion and clarification.

== Section 10.7 Defines can't use inherits for parameter defaults

Should this be reduced to only cover classes, or should the description be expanded to cover the style of setting defined resource defaults also? (Which happen to be the "bad" example; whoops.)

The guide's preferred form works for defined types, too, provided the default values are static (but then why would you even consider the other style?).  On the other hand, the guide's preferred style does not work even for classes when a parameter's default value needs to be computed from the actual values of other parameters.   I think the guideline should be softened to a recommendation, its applicability narrowed to classes only, and the example labels changed from "GOOD" and "BAD" to something like "Preferred" and "Discouraged".  The non-applicability to defined types should be explicitly called out, for clarity.

 

== Section 11.2 Recommendation against using "include" is unclear or harmful

I think this was to describe how an over-zealous use of `include` may cause classes to be declared with the default parameters before a user may declare them with parameters. Or it could have been about cross-module class inclusion in public modules. What should we do with this section?

I don't think there is any over-zealousness problem associated with using 'include', unless it is with respect to declaring classes that are not directly required in the scope of the declaration.  Needed classes should be declared, and that generally should be done via an 'include', 'require', or 'contain' call.  (And note, too, that 'contain' and 'require' have all the same declaration semantics as 'include', but the current text ignores them.)

In fact, I think the guide is almost exactly backward on this matter.  The style points that should be recommended are:

• Classes and defined types should declare the classes on which they rely
• Data should generally be bound to classes via Puppet's Hiera-based automatic data-binding facility, instead of via resource-style class declarations.  In particular,
  
• In modules, the resource style should never be be used to declare public classes of that or any other module.
• Everywhere, the 'include', 'require', and / or 'contain' functions should be used instead of resource-style class declarations if at all possible.

All that can be well justified based on Puppet's design and implementation, though I grant that "if at all possible" is pretty wishy-washy.  I'd go further with a style guide for my own site, but the things I'd add would be a bit more controversial.

 

= Proposals:

If the below changes sound good, then we'll go with that.

== Section 5 line length is an issue

Managing line lengths in puppet is difficult. This point should change to describe where to do syntax-wise linebreak rather than character-wise breaks (ie, any hashes containing more than a single key/value pair, etc.).

Yes.

I'd be ok even with setting a soft bound on line length -- e.g. a recommendation / preference that line lengths not exceed (say) 140 characters if it can reasonably be avoided.  Emphasize that this is a readability issue.

 

== Section 9.6 symbolic modes

This was answered in that symbolic modes can perform operations that numeric modes cannot. The style guide does not dictate which form to use, so this should not be an issue.

Ok.

 

== Section 10.6 No required class parameters

Some classes just MUST have required parameters and there is no way around it. The style guide should be modified to include "should minimize the number of required parameters" as a style recommendation.

Ok.

 

== Section 10.2 item 7, resource ordering alphabetically

Remove the mention of ordering so that it is simply "Should declare resources." This also impacts section 9.4 and should be reconciled.

Thus the item would designate a location for resource declarations as a group, relative to other statements, but it would not recommend any particular ordering of the resource declarations with respect to each other.  That works for me.

 

== Section 10.2 includes vs validation first

Validation cannot come at the end of the file due to parse order, but it is possible to allow includes to come before validation, so we should just not dictate this bit of ordering.

(This issue will go away entirely after the release of puppet 4, as validation may happen inline with the parameters themselves due to the typing system.)

The option to have validation after class declarations works for me.  It may, in fact, be necessary under certain circumstances -- for example, when validation of an item depends on class variables of another class.

 

== Section 10.2 Unclear description of "items 7 & 8"

I'm not exactly sure what this is describing either. Most likely superfluous and will be removed.

Good.

 

== Section 10.4 Chaining arrow syntax with only references, not declarations

Agreed. And perhaps that if there are line breaks around arrow syntax, they must only happen to the right of the arrow and not the left so that arrows are not "hidden" at the end of previous lines.

I'm missing it: how does allowing line breaks only to the right of a chain operator prevent it from being hidden at the end of a line?  Shouldn't that be "left"?

Chaining resource declarations doesn't actually bother me, personally.  I won't particularly resist the style guide discouraging that, but I'd be at least as well satisfied by a softer guideline.  For example, "a chain operator should appear on the same line as its right-hand operand."  Or the still-softer "a chain operator should appear either on the same line as its right-hand operand or on a line by itself."

 

== Section 12.1 $unique_name = $name is unclear

I believe this was to describe how the continued use of $name throughout a define can lead to confusion, as $name has no strong semantic meaning. Thus a "good" example would be https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/listen.pp#L2 and a bad example would be... https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/vhost.pp (because $name is scattered throughout the define and has no definite meaning).

I don't think "unique_name" has any more or less clear of a meaning in such contexts than does "name".  Any lack of clarity is about which entity the name belongs to, and describing it as "unique" doesn't really help with that.

Moreover, I observe that what you're saying the section is about is not at all how I read it.  (Lack of clarity? Check.)  I don't think we can talk about the wording details until we settle more generally what the section is trying to say.  If you're confident about that (you don't sound it) then we can discuss the wording, but at this point my position would be that the section should just be deleted.

 

== Section 18 Parameter ordering unclear

The paragraph about ordering should be moved to 10.6 or removed altogether.

Section 18 should be removed completely.  The part about parameter ordering is already adequately covered by section 10.6, and the rest has nothing to do with (code) style guide, nor even module design / layout.

 

== Section 19 should be moved to Section 2

Because if becoming a specification for puppet-lint is a purpose, then section 2 is best.

That supposes, of course, that the document is intended to be used as a reference for the style to be checked by puppet-lint.  If that's indeed so then yes, put that in section 2 (and word it more appropriately).

 

== "Style guide" vs "specification" discussion from jcbollinger

To paraphrase, "the 'language style guide' is written as a technical specification covering more than the language; it covers module structure and module contribution conduct. This is not 'style' and is beyond the scope of this document."

In answer... we should change the title to something other than "The Puppet Language Style Guide" :). I personally feel that "Module Style Guide" would still encompass what is included here, when the above edits are taken into account.

A bona fide style guide is appropriate and wanted.  Moreover, it is inappropriate to present a document that purports to be a general specification for how all modules must be implemented, when in fact there are many viable alternatives.  PL has a lot of standing in this area, but not so much as to lay down hard general rules like that.

Instead of recasting the current document as something else, it should be split into at least two documents.  One would be a true style guide (or even a "manual") describing PuppetLabs's official style for DSL code.  Key there would be that any "must", "must not", or equivalent in it be explicitly in the context of "in order to comply with the style described in this document".  Another document might be a module design and layout specification, perhaps for enforcement on modules published on the Forge.  This second document might require the modules to which it pertains to comply with the style described in the separate style guide, so as to have the effect (in its scope) of a combined document.

John

[Hunter Haugen]

== Section 10.7 Defines can't use inherits for parameter defaults

Should this be reduced to only cover classes, or should the description be expanded to cover the style of setting defined resource defaults also? (Which happen to be the "bad" example; whoops.)

The guide's preferred form works for defined types, too, provided the default values are static (but then why would you even consider the other style?).  On the other hand, the guide's preferred style does not work even for classes when a parameter's default value needs to be computed from the actual values of other parameters.   I think the guideline should be softened to a recommendation, its applicability narrowed to classes only, and the example labels changed from "GOOD" and "BAD" to something like "Preferred" and "Discouraged".  The non-applicability to defined types should be explicitly called out, for clarity.

Okay.

== Section 11.2 Recommendation against using "include" is unclear or harmful

I think this was to describe how an over-zealous use of `include` may cause classes to be declared with the default parameters before a user may declare them with parameters. Or it could have been about cross-module class inclusion in public modules. What should we do with this section?

I don't think there is any over-zealousness problem associated with using 'include', unless it is with respect to declaring classes that are not directly required in the scope of the declaration.  Needed classes should be declared, and that generally should be done via an 'include', 'require', or 'contain' call.  (And note, too, that 'contain' and 'require' have all the same declaration semantics as 'include', but the current text ignores them.)

In fact, I think the guide is almost exactly backward on this matter.  The style points that should be recommended are:

• Classes and defined types should declare the classes on which they rely
• Data should generally be bound to classes via Puppet's Hiera-based automatic data-binding facility, instead of via resource-style class declarations.  In particular,
  
• In modules, the resource style should never be be used to declare public classes of that or any other module.
• Everywhere, the 'include', 'require', and / or 'contain' functions should be used instead of resource-style class declarations if at all possible.

All that can be well justified based on Puppet's design and implementation, though I grant that "if at all possible" is pretty wishy-washy.  I'd go further with a style guide for my own site, but the things I'd add would be a bit more controversial.

Well put :). Re-reading this section makes me think that it was historically about the "declare variables then include classes" pattern that parameterized classes were created to obviate, but got mashed together with a design pattern for cross-module class inclusion like in a game of telephone. The whole "non-deterministic scoping" thing is probably irrelevant at this point.

 

== Section 10.4 Chaining arrow syntax with only references, not declarations

Agreed. And perhaps that if there are line breaks around arrow syntax, they must only happen to the right of the arrow and not the left so that arrows are not "hidden" at the end of previous lines.

I'm missing it: how does allowing line breaks only to the right of a chain operator prevent it from being hidden at the end of a line?  Shouldn't that be "left"?

Whoops yes, to the left of the arrow. Perhaps I was thinking "with the arrow to the right of the linebreak" when I wrote that. IE, https://github.com/puppetlabs/puppetlabs-mysql/blob/3.1.0/manifests/server.pp#L64-L72 is a bad example as the linebreaks are on the right of the arrow :).

But anyway... V

 

Chaining resource declarations doesn't actually bother me, personally.  I won't particularly resist the style guide discouraging that, but I'd be at least as well satisfied by a softer guideline.  For example, "a chain operator should appear on the same line as its right-hand operand."  Or the still-softer "a chain operator should appear either on the same line as its right-hand operand or on a line by itself."

I like this more than talking about left/right linebreaks.

== Section 12.1 $unique_name = $name is unclear

I believe this was to describe how the continued use of $name throughout a define can lead to confusion, as $name has no strong semantic meaning. Thus a "good" example would be https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/listen.pp#L2 and a bad example would be... https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/vhost.pp (because $name is scattered throughout the define and has no definite meaning).

I don't think "unique_name" has any more or less clear of a meaning in such contexts than does "name".  Any lack of clarity is about which entity the name belongs to, and describing it as "unique" doesn't really help with that.

Moreover, I observe that what you're saying the section is about is not at all how I read it.  (Lack of clarity? Check.)  I don't think we can talk about the wording details until we settle more generally what the section is trying to say.  If you're confident about that (you don't sound it) then we can discuss the wording, but at this point my position would be that the section should just be deleted.

I do know what I want, but I'm not a great wordsmith :(. The point of the section was to say that "$name in a define doesn't have a semantic meaning, so if you're going to use $name for anything programatic then assign it to a semantically-meaningful variable for clarity's sake and use that instead." But if you think that's not really a useful pattern then we could delete the section.

I guess the best I can do is give the two examples linked above. If you want better words for it, I'll have to defer to Lauren.

  

== "Style guide" vs "specification" discussion from jcbollinger

To paraphrase, "the 'language style guide' is written as a technical specification covering more than the language; it covers module structure and module contribution conduct. This is not 'style' and is beyond the scope of this document."

In answer... we should change the title to something other than "The Puppet Language Style Guide" :). I personally feel that "Module Style Guide" would still encompass what is included here, when the above edits are taken into account.

A bona fide style guide is appropriate and wanted.  Moreover, it is inappropriate to present a document that purports to be a general specification for how all modules must be implemented, when in fact there are many viable alternatives.  PL has a lot of standing in this area, but not so much as to lay down hard general rules like that.

Instead of recasting the current document as something else, it should be split into at least two documents.  One would be a true style guide (or even a "manual") describing PuppetLabs's official style for DSL code.  Key there would be that any "must", "must not", or equivalent in it be explicitly in the context of "in order to comply with the style described in this document".  Another document might be a module design and layout specification, perhaps for enforcement on modules published on the Forge.  This second document might require the modules to which it pertains to comply with the style described in the separate style guide, so as to have the effect (in its scope) of a combined document.

Thanks for the clarification. We'll take this into account for the next major revision. 

[jcbollinger]

On Wednesday, February 4, 2015 at 11:39:56 AM UTC-6, Hunter Haugen wrote:

 

== Section 12.1 $unique_name = $name is unclear

I believe this was to describe how the continued use of $name throughout a define can lead to confusion, as $name has no strong semantic meaning. Thus a "good" example would be https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/listen.pp#L2 and a bad example would be... https://github.com/puppetlabs/puppetlabs-apache/blob/1.2.0/manifests/vhost.pp (because $name is scattered throughout the define and has no definite meaning).

I don't think "unique_name" has any more or less clear of a meaning in such contexts than does "name".  Any lack of clarity is about which entity the name belongs to, and describing it as "unique" doesn't really help with that.

Moreover, I observe that what you're saying the section is about is not at all how I read it.  (Lack of clarity? Check.)  I don't think we can talk about the wording details until we settle more generally what the section is trying to say.  If you're confident about that (you don't sound it) then we can discuss the wording, but at this point my position would be that the section should just be deleted.

I do know what I want, but I'm not a great wordsmith :(. The point of the section was to say that "$name in a define doesn't have a semantic meaning, so if you're going to use $name for anything programatic then assign it to a semantically-meaningful variable for clarity's sake and use that instead." But if you think that's not really a useful pattern then we could delete the section.

You're not making sense.  If $name has no semantic meaning in a given context, then it is no more useful for initializing some other variable's value than for anything else.

I do acknowledge that there is sometimes confusion about what $name means inside a defined type body.  Typically that takes the form of thinking that within a resource declaration inside the defined type, $name would refer to the contained resource's name rather than to the name of the containing instance of the defined type.  The proposed style point does not help with that particular issue, as it's more about scoping than about choice of variable names.

Creating an alternative variable for the instance name is not particularly useful for many simple defined types, such as many that just wrap a plugin type.  On the other hand, the stylistic pattern may be helpful in more complex defined types.  In such cases, the alternative variable name I would choose for the instance name within a defined type 'module::foo' would be "foo_name".  That at least addresses the main point of confusion (what is named), though it still does little for the scoping confusion (because confused users would still try to use $name).  You could offer that pattern as a suggestion, but I don't think it should be a rule.

As long as we're on defined type automagic variables, though, consider that there is also $title, which will always have the same value as $name for a defined type.  It would be entirely reasonable for a style rule to designate one of those to always be used instead of the other.  Absent such a rule, all the above considerations regarding $name also apply to $title.


John

[Lauren R]

A big thanks to everyone for all the feedback! It's pretty awesome to work on a project that people care so much about.

Here's what's happening:

We gathered all the comments, suggestions, and opinions in a document and walked through each one. Some small things (that completely block understanding) are being updated now (expect a z release early next week). The bigger things were logged in a public ticket and will be addressed in the next version of the guide, slated to come out sometime after Puppet 4.0 launches. 

Again, thank you for all the discussion, pull requests, and insight. Puppet people are some of my favorite people ever.

Track the updates ticket here: https://tickets.puppetlabs.com/browse/DOCUMENT-266

Best,

Lauren

[Trevor Vaughan]

When I think of $name, I define it as this: https://docs.puppetlabs.com/guides/custom_types.html#namevar

In other words: I need a unique string, this is it and I'll use it where I need uniqueness so that my resources don't collide.

But, as is pointed it out, it is actually meaningless beyond that usually.

So, $unique_string = $name would be as accurate as I could make it in many cases.

I would just make sure people know what it's for and don't make any assumptions about it whatsoever.

Trevor

--
You received this message because you are subscribed to the Google Groups "Puppet Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-users...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-users/f623ece2-cd9a-45f0-9a7a-ae3a4d758e2a%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Gareth Rushgrove]

Worth noting that much of the validation is type checking, and with
actual real types coming in 4.0 I'm looking forward to deleting much
of those lines of validate_ function calls.

http://puppet-on-the-edge.blogspot.co.uk/2014/06/optionally-typed-parameters.html

Gareth

> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Users" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to puppet-users...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/puppet-users/CANs%2BFoWQ3LhkzndGmgzSVt%3D1xR%2BjpUFj0SPWuftS65PNSJWX_Q%40mail.gmail.com.

>
> For more options, visit https://groups.google.com/d/optout.



--

Gareth Rushgrove
@garethr

devopsweekly.com
morethanseven.net
garethrushgrove.com