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 *un*justified, 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
--
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 [email protected].
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.
