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 On Tue, Feb 3, 2015 at 8:54 AM, jcbollinger <john.bollin...@stjude.org> wrote: > > > 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 puppet-users+unsubscr...@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 > <https://groups.google.com/d/msgid/puppet-users/011324f7-4983-4baf-891d-e8b8a72f7a8c%40googlegroups.com?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/d/optout. > -- 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/CAJaQvGAUmOHEtKnCuyqTRMSbRD5fzaUTiaaqcpkXocfM4_-8Tg%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
