Skip to content
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

(FACT-932) Add new function, fact() #787

Merged
merged 3 commits into from
Jul 21, 2017
Merged

Conversation

reidmv
Copy link
Contributor

@reidmv reidmv commented Jun 30, 2017

This PR doesn't solve for FACT-932, but is related to it and discussion
there.

The fact() function allows dot-notation reference to facts. It is an
alternative to using $facts directly with array-indexing. Array-indexing
is often onerous to use since it doesn't align with how structured facts
are accessed elsewhere in the ecosystem and if any element in a
multi-step path doesn't exist, array indexing can cause a compilation
failure.

Example usage:

fact('os.family')

@ripienaar
Copy link

Would be nice to split this into 2 functions. One to dig like this into any hash and then use that in the fact function.

@reidmv
Copy link
Contributor Author

reidmv commented Jun 30, 2017

@ripienaar any ideas for what to call the dig-like part? dotdig()?

@ripienaar
Copy link

I'm not a good guy to ask about naming things :P core has dig(), I am not sure at all

@reidmv
Copy link
Contributor Author

reidmv commented Jun 30, 2017

I looked briefly at just splitting this into two functions and it looked like doing that might make it harder to keep the error messages specific to the fact-lookup use case.

It could be possible to extend the existing function with a two-argument dispatch, such that it could be called for other arbitrary arrays, but when called with only one argument it behaves as it does today.

@ripienaar is there a specific use case you're thinking of for having the function split? As you mentioned dig() exists in core and allows a form of this lookup; what's specific to this function is the dot-separated-value notation used by Facter.

Would the ability to call fact() with a hash solve the use case(s) you're thinking of? E.g.

$simple = fact('os.family')
$arbitrary = $alternative.fact('os.family')

The fact() function allows dot-notation reference to facts. It is an
alternative to using $facts directly with array-indexing. Array-indexing
is often onerous to use since it doesn't align with how structured facts
are accessed elsewhere in the ecosystem and if any element in a
multi-step path doesn't exist, array indexing can cause a compilation
failure.

Example usage:

    fact('os.family')
Because sometimes people want to use an alternative data set, but treat
it like it's a set of facts.
@ripienaar
Copy link

The dot notation is just quite nice and now quite pervasive throughout so the use case is just to use it on other hashes - I wish core dig was this and I think I remember asking for that when it was made. Alas now it's just odd

Naming seems off to be able to pass a extra parameter to something called fact to them dig into another non fact thing to me

@reidmv
Copy link
Contributor Author

reidmv commented Jul 3, 2017

@ripienaar I think I agree with you on the name. I had a good time for a minute writing up a commit to allow using alternative facts, but in the end I think it's probably best for now to keep things simple and let fact() be single-purpose for looking up facts with no potentially confusing extensions. The only use case where the name makes sense is where it's being used on the $facts hash, and I think we'd rather in that circumstance promote the raw use of fact().

$facts.fact('os.family')

Reverting in the PR for now, but leaving the commit in the history so we can still look at it conversationally if we want to.

This reverts commit 409a974.

After thinking about this, use case, and function naming, I think it's
probably best for now to keep things simple and let `fact()` be
single-purpose for looking up facts with no potentially confusing
extensions. The only use case where the name makes sense is where it's
being used on the `$facts` hash, and I think we'd rather in that
circumstance promote the raw use of `fact()`.
@reidmv
Copy link
Contributor Author

reidmv commented Jul 3, 2017

What's up with the Appveyor build failures? They don't look related to this PR at all.

@tphoney
Copy link
Contributor

tphoney commented Jul 6, 2017

@reidmv i will look into the appveyor failure it smells bad to me

@binford2k
Copy link
Contributor

@ripienaar for the record, as part of the conversation that @reidmv and I had prompting this PR, I wrote exactly that function and proposed it in a different PR #759

If there is value in a .value() function, then we can certainly combine them.

@binford2k binford2k mentioned this pull request Jul 21, 2017
@hunner hunner merged commit 0f35700 into puppetlabs:master Jul 21, 2017
hunner added a commit that referenced this pull request Jul 21, 2017
(FACT-932) Add new function, fact()
@npwalker
Copy link
Contributor

@reidmv do we need another PR to document this new function in the README?

@reidmv
Copy link
Contributor Author

reidmv commented Jul 21, 2017

@npwalker Doh! Yes, we do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants