On Jul 27, 2009, at 9:52 AM, Steven Jenkins wrote:
>
> Luke Kanies wrote:
> ...
>>> + # Deal with backups.
>>> + def handlebackup(file = nil)
>>> + # let the path be specified
>>> + file ||= self[:path]
>>> + return true unless FileTest.exists?(file)
>>> + # if they specifically don't want a backup, then just say
>>> + # we're good
>>> + return true unless self[:backup]
>>> + unless self.bucket || self[:backup]
>>> + self.err "Need a bucket or a backup setting for
>>> backups"
>>> + return false
>>> + end
>>
>> This code isn't really necessary, is it? We already return
>> if :backup
>> isn't set, which should cover all of the bases, since the 'bucket'
>> value can only be derived from :backup.
>>
>
> I left that code in the handlebackup method in case setup code for
> 'we have a
> filebucket, but we set 'backup' to false); however, that's getting
> normalized
> earlier in the process (e.g., at File.new).
>
> I don't think that leaving the explicit checks in is bad, though, as
> defensive
> programming is a good idea in general.
If we are confident of the validation of the :backup value, and I
think we are, then I think we should leave this out. The only real
input, other than the file name, to this method is a result of
the :backup setting, so we are closely coupled to it already.
>
>>> + return handlebucket(file) if self.bucket
>>> + return handlebackuplocal(file, self[:backup])
>>
>> Is it reasonable to get rid of the 'handle' term here? I think I
>> did a
>> poor job of naming the method in the first place; I used to have a
>> tendency to use generic verbs all over the place -- evaluate, handle,
>> etc. It probably even makes sense to just change the 'handlebackup'
>> entry point to be just 'perform_backup' or something similar. And
>> while most old code doesn't do it, I've been working toward more
>> common naming methods: Longer method names, using underscores.
>>
>
> Sure. s/handle/perform_/ is not a problem.
Great.
>
>>> + end
>>> +
>>> + private
>>> +
>>> + def handlebucket(fileobj)
>>> + file = (fileobj.class == String) ? fileobj : fileobj.name
>>> + case File.stat(file).ftype
>>> + when "directory"
>>> + # we don't need to backup directories when recurse is
>>> on
>>> + return true if self[:recurse]
>>> + info "Recursively backing up to filebucket"
>>> + require 'find'
>>> + Find.find(file) do |f|
>>> + if File.file?(f)
>>> + sum = self.bucket.backup(f)
>>> + self.info "Filebucketed %s to %s with sum %s" %
>>> + [f, self.bucket.name, sum]
>>> + end
>>> + end
>>> + return true
>>> + when "file"
>>> + sum = self.bucket.backup(file)
>>> + self.info "Filebucketed %s to %s with sum %s" %
>>> + [file, self.bucket.name, sum]
>>> + return true
>>> + when "link"; return true
>>> + end
>>> + end
>>
>> I'd still like to see this method broken into two methods: One with
>> the case statement, and one to do the backing up.
>
> I'm not sure I follow you here. If we move the actual backing up
> into a
> separate method, the current 'handlebackup' method is only going to
> call that
> method, so it's purpose is unclear to me.
As I mentioned in my first comment, you would start with a method like
this:
def backup_with_filebucket
ftype = stat().ftype # this is part of the file type already
case ftype
when "directory"
Find.find(self[:path]) { |f| backup_file_with_filebucket(f) if
File.file?(f) }
when "file"; backup_file_with_filebucket(f)
when "link"; return true
end
end
And then you have:
def backup_file_with_filebucket(f)
sum = self.bucket.backup(f)
self.info "Filebucketed %s to %s with sum %s" % f,
self.bucket.name, sum]
end
Should end up being a bit shorter, and at worst won't be any longer,
and you now have two simple methods: One that does the branching, and
one that does the work. You've also removed the small amount of code
duplication (the part that actually calls out to the filebucket).
>
> On the other hand, I can see having two new methods: one to handle
> directories
> and one to handle files, and having perform_bucket call those, but
> the current
> method is fairly small, so breaking it up into even smaller pieces
> doesn't seem
> to be a win.
I think the current method is a combination of enough length and
enough nesting that it's hard to read, and it's certainly hard to see
that the heart of the method -- calling out to the filebucket client
-- is actually duplicated (as it was in the original code).
>
> Let me know how you'd like to proceed on this.
> ...
>>> +
>>> +describe Puppet::Util::Backups do
>>> + describe "when backing up a file" do
> ...
>> We should be moving all backup-related tests from the file tests into
>> this test module. Initially just copy them, make sure they all still
>> work, and once you're confident in the new tests, just add a test to
>> the file tests to verify this method gets called appropriately and
>> remove the old tests.
>>
>
> Will do.
>
>>> + end
>>> +end
>>
>> For the tests, you can make a simple test class that skips all of the
>> normal complexity:
>>
>> class BackupTesting
>> include Puppet::Util::Backups
>> end
>>
>> describe Puppet::Util::Backups do
>> before do
>> @backup = BackupTesting.new
>> end
>> ...
>> end
>>
>> Then you can skip all of the interactions with the type and focus on
>> the behaviour of this module.
>
> You're still going to have to do
>
> SomeType.new
>
> Going via the File type is at least close to how the code is used in
> practice.
> I see that as a toss-up decision.
That's true.
>
>> I'm even considering whether it makes
>> sense to pass the needed information to the method, so the file can
>> call it something like this:
>>
>> perform_backup(self[:path], self[:backup], self.bucket)
>>
>> I think that's overkill in this case, but it's appealing.
>>
>
> Methods that take parameters rather than depend on implicit types
> are easier to
> understand; however, from what I can tell, that is far more
> widespread than this
> part of the code. I suggest doing that refactoring as a different
> commit: e.g.,
> do a single commit that does that normalization throughout an entire
> directory;
> this commit already mixes far more changes than I'd normally suggest
> in a single
> patch.
I think leaving the method signature like it is makes sense; it was
just something I was considering.
--
Progress isn't made by early risers. It's made by lazy men trying to
find easier ways to do something. --Robert Heinlein
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
