On Sun, Oct 24, 2010 at 5:13 PM, Daniel Pittman <[email protected]> wrote:
> Markus Roberts <[email protected]> writes: > > >> > Fix is_virtual fact to return strings rather than bools. > >> > >> Wouldn't a better way to achieve this be this patch? > >> > >> diff --git a/lib/facter/util/fact.rb b/lib/facter/util/fact.rb index > >> e78ed97..e01833b 100644 --- a/lib/facter/util/fact.rb +++ > >> b/lib/facter/util/fact.rb @@ -77,6 +77,9 @@ class Facter::Util::Fact > > >> break tmp unless tmp.nil? or tmp == "" } > >> > >> + # Fact values must necessarily be strings, enforce that. + > > >> @value = @value.to_s + unless foundsuits > > >> Facter.debug "Found no suitable resolves of %s for %s" % > [[email protected], > >> @name] end > > > > Yes but... > > > > 1) I'd still suggest my suggested simplification on the original code > (but > > your approach makes it off topic). > > 2) > > ENOCOMMENT? (I agree that your suggested code is much, much nicer.) > > >> Otherwise the uninitiated might make the sad and painful discovery of > this > >> themselves by returning false and having to learn that only '' is > actually > >> "not true" as far as puppet is concerned. > > > > If I'm reading this correctly neither patch will work, as both result in > > "false" rather than "". > > Uh-huh. I would rather that facter returned rich types, ideally through > JSON[1], since that gives us a solid and language-independent way to > express a > rich set of basic data types without over-much complexity. > I think we all want rich types in Facter, and I believe we've scheduled it for 2.0. Those of you who saw Paul Nasrat talk at Puppet Camp would have heard him plead for more help on Facter. It's a really easy way to contribute to the Puppet ecosystem, as it's a small and easily understandable code base. That being said, we do need to fix is_virtual to return strings before we get other types. > > However, at least now there is more clarity, and adding a warning on > conversion would improve that - I would have known facter was unhappy with > my > boolean false sooner. > > (...and a real patch deserves that warning, I think, since this is a hard > requirement and being too liberal in what we accept here leads to problems > for external developers later, IMO) > I concur. I made the mistake of using booleans early on, and it would have been useful to have this made obvious before I dug a larger hole for myself. > > > 3) Looking at the surrounding (gawd-awful) code I'm not convinced this is > > exactly the right spot for it...but that > inject-with-next-unless-and-break > > pattern makes my head hurt. It looks as if this change would > sometimes > > stop the "value for %s is still nil" branch from firing. Maybe it > > should be done right before the "return @value"? > > I don't know the patch is entirely correct; it was an RFC more than a > serious "I actually bothered to check facter still works" submission. > > So, yeah, that would need more attention than I gave it. > > Daniel > > Footnotes: > [1] ...with enforcement of the UTF-8 encoding requirement for the text. :) > > -- > ✣ Daniel Pittman ✉ [email protected] ☎ +61 401 155 > 707 > ♽ made with 100 percent post-consumer electrons > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- Nigel Kersten Product Manager, Puppet Labs http://www.puppetlabs.com Twitter: @nigelkersten -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
