Luke Kanies wrote: > On Jul 24, 2009, at 9:43 AM, Steven Jenkins wrote: > ... > > Both of these can be trimmed up a bit using this idiom: > > return true unless FileTest.exists?(file) >
Yes, I changed one of the tests and then reminded myself of 'one change at a time' so didn't do the others. I'll happily go back in and reclean that up. >> + backup = self.bucket || self[:backup] > > This line should be removed; I'm pretty sure this value isn't used > anywhere. Actually it is used two different places, once as a method, once as a variable, and I see a bug in what I did where it is used as a variable...easily fixed. ... > Can you change all of the notice logs here to info? They really > shouldn't be at the normal log level; I've had a customer complain > about that. It's an unrelated problem, but this method seems to be > the main culprit. > Sure. >> + end >> + end >> + >> + return true >> + elsif self[:backup] >> + handlebackuplocal(file) >> + else >> + self.err "Invalid backup type %s" % >> backup.inspect >> + return false > > This branch can be removed because we're doing the validation in the > parameter - we know we'll only ever have valid values here. > Having an else clause is good practice (otherwise, something slips through silently). I'll remove it, though, and make sure there is an adequate check at the top of the method instead (e.g., 'fail early'). >> + end >> + when "link"; return true >> + else >> + self.notice "Cannot backup files of type %s" % >> File.stat(file).ftype >> + return false >> + end >> + end > > I think this whole method would be better off refactored into a > dispatch-based structure instead of the case statements. Something > like: > > return backup_with_filebucket if self.bucket > return backup_with_file Good point. Will do. ... > > This should allow all of the methods to be very short and easily > testable. > > Speaking of tests, there, um, don't appear to be any. :) I'm most concerned about the existing failing test at the moment, so didn't want to confuse the issue by other tests. Will add spec tests for the module, though. > ... > There seem to be some weird indentation issues here -- are you using 4 > spaces throughout? > >> + end >> + >> +end >> -- >> 1.6.3.3 >> Apparently in some cut-and-paste things got garbled. Will clean that up (and make sure .vimrc is set right for Puppet). Thanks, 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 -~----------~----~----~----~------~----~------~--~---
