-
Notifications
You must be signed in to change notification settings - Fork 66
(MODULES-9428) Clarify composite namevar edgecases and add examples #140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of suggestions but otherwise looks good (after the typos that Henrik spotted are fixed).
I'll come back to this after contributor summit. |
7cfceec
to
54d0eea
Compare
I've found a few more tickets that fold into the composite namevars issue that I'd like to bundle into this fix:
PDK-1220 is already covered in this text, the others need a bit more consideration. against my usual preferences in this repo, I've rebased and munged the commits on this change as far as it went to separate the editorial and the content changes. |
54d0eea
to
1ea497b
Compare
Break up the dense information from the paragraph into a bulleted list. This ensures that the different statements do not run into each other.
Improve the initial type schema example.
…e less duplication
* Point out that there can be more than two namevars * Improve composite namevar example This is rewording doc strings, fixing syntax errors, and reformatting the example. * Clarify behaviour when no `title_pattern` is specified Make sure to communicate the behaviour and consequences when no `title_patterns` are specified. * Upgraded the "order" note to a definitive statement on the matching behaviour. * Update the description for `get` and `set` for composite namevars This moves the existing explanation for how `get` should behave to the definition of the method and adds examples for the values passed around by `get` and `set`. * Mention again that composite namevars are supported by SimpleProvider * Remove the `title` key from the hash being passed to SimpleProvider methods Since we can't guarantee to always have a title available, we can't promise to pass one through to the implementation. * Add a note and examples about `names` usage in `simple_get_filter` This also covers PDK-1219, PDK-1220, and PDK-1221.
1ea497b
to
8df5350
Compare
@Lavinia-Dan @da-ar this is now in a shape to review. Note that the commits are broken up into distinct chunks that should help reduce clutter during reviewing. |
The remote_resource example had remnants from a supports_noop implementation. This change removes those remnants as they are misleading and do not add value to this example.
* Fixed quoting in examples * Improve wording in a couple of places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from me
After conversation with Henrik, I'm certain that we should not burden the text or code with the complexity of having title_patterns optional.
Ok, I think this wraps it up. The implementation is available at puppetlabs/puppet-resource_api#174 and there don't seem to be further comments here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one minor comment. The rest reads clearly.
No description provided.