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
--
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+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-users/e08e3b12-24ba-4df1-9005-8988d058faa5%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
