On Jun 8, 2009, at 9:15 AM, Paul Nasrat wrote:
>
> 2009/6/8 Felix Schäfer <[email protected]>:
>
>>
>> And an assignment is exactly what I was aiming for: the assignment
>> "fails" if the right hand doesn't exist, which is the condition in
>> which I don't want to go into the branch, if the right hand exists, I
>> get the assignment "for free" (the only pitfall here would be if a
>> possible value of the assignment was 'false', which shouldn't
>> happen).
>
> I personally find that style to be more confusing as the intent isn't
> as clear - I got misled instantly!
I actually use this constantly, and I find it both more readable and
more maintainable.
>
> I'm not sure we need to be worrying about the optimisation here, and
> I'd tend to fall on the side in favour of clarity.
>
> Reading over the existing fact, it looks like we're trying to be too
> generic, we shouldn't even try /proc/uptime on Solaris and I assume
> AIX too.
>
> I think we might be better here with splitting this out into two
> confined resolvers, one for linux systems with /proc/uptime (which has
> been around for a long time) and one for non-linux systems. I think
> this will achieve the only do things once desire of your bug report
> and clean up the code at the same time.
>
> eg:
>
> Facter.add(:uptime) do
> confine :kernel => %w{SunOS AIX}
> setcode do
> #generic uptime command processing
> end
> end
>
> Facter.add(:uptime) do
> confine :kernel => :linux
> setcode do
> #/proc/uptime parsing and manipulation
> end
> end
> end
That seems reasonable.
>
>> If you prefer to separate this into two distinct steps, I can rework
>> it accordingly.
>
>>> White space noise pollution.
>>
>> Yeah, well, I'm often a little maniac regarding needless white space
>> at the end of lines and so on, I'll try to keep it in bounds next
>> time
>> around, sorry.
>
> It's ok just do it as a seperate commit rather than with the logic,
> this is purely so that code reviewers can focus on discreet changes.
>
>> Regarding the complex logic: I wasn't sure if it should go into
>> facter/
>> uptime or facter/util/uptime, my gripe with the later being getting
>> bounced 3 or 4 times around the 2 files.
>
>> Anyway, this brings me to a question I still wanted to discuss,
> ..
>> It seems I have diverged a little bit here. If the answer is longer
>> than "already discussed" or "won't do because", please move to a new
>> thread.
>
> The discussion is fine - we need to discuss the design of things more.
> I'll follow up on that point in a separate mail so it doesn't get lost
> in the patch discussion.
>
>> A last word concerning the patch: I'm trying to get up to speed on
>> ruby as well as on facter, and I'd like to help facter/puppet in the
>> process, I'd appreciate a little more constructive comments.
>
> Sure, apologies if I was a little abrupt I just did a pass through
> review. Unfortunately some of the facts are a little messy
> and I'm trying to get to code with a better level of abstraction as we
> move to 1.6 and 2.0. We're always grateful to receive patches, sorry
> that didn't come across.
>
> Maybe some better contributor focused docs for facter would be useful,
> it's on my TODO and I'll try to get to this week.
That'd be great.
--
Trying to determine what is going on in the world by reading
newspapers is like trying to tell the time by watching the second
hand of a clock. --Ben Hecht
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
