Skip to content

(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

Merged
merged 11 commits into from
Jun 28, 2019

Conversation

DavidS
Copy link
Contributor

@DavidS DavidS commented May 24, 2019

No description provided.

@DavidS DavidS requested review from da-ar and clairecadman May 24, 2019 13:27
Copy link
Contributor

@clairecadman clairecadman left a 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).

@DavidS
Copy link
Contributor Author

DavidS commented May 29, 2019

I'll come back to this after contributor summit.

@DavidS DavidS force-pushed the clarify-composite-namevars branch from 7cfceec to 54d0eea Compare June 11, 2019 14:02
@DavidS
Copy link
Contributor Author

DavidS commented Jun 11, 2019

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.

@DavidS DavidS changed the title Clarify composite namevars (MODULES-9428) Clarify composite namevar edgecases and add examples Jun 18, 2019
@DavidS DavidS force-pushed the clarify-composite-namevars branch from 54d0eea to 1ea497b Compare June 18, 2019 19:49
DavidS added 7 commits June 19, 2019 11:14
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.
* 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.
@DavidS DavidS force-pushed the clarify-composite-namevars branch from 1ea497b to 8df5350 Compare June 19, 2019 10:23
@DavidS
Copy link
Contributor Author

DavidS commented Jun 19, 2019

@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
Copy link

@da-ar da-ar left a 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.
@DavidS
Copy link
Contributor Author

DavidS commented Jun 25, 2019

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.

Copy link

@da-ar da-ar left a 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.

@DavidS DavidS merged commit 51492fe into puppetlabs:master Jun 28, 2019
@DavidS DavidS deleted the clarify-composite-namevars branch June 28, 2019 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants