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. >> + 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. >> + 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. 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. 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. > 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. Steven Jenkins End Point Corporation --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
