On Nov 15, 2010, at 1:25 PM, Jesse Wolfe wrote: > While I certainly appreciate your willingness to get your hands dirty with > this code, I hesitate to accept a patch that doesn't produce any change in > functionality. > Some of your edits are things we generally agree on: removing unused > variables, removing the unnecessary subclassing of Exception; > but many of the others are debatable: not everyone agrees that "unless" is > more readable than "if not", I'm not sure that DSLOCAL_DIR is the best way to > extract that directory name, and we pretty consistently use "self.method" > rather than "class << self" in the puppet codebase. > > There's no official puppet policy on this, as far as I know, but I'm inclined > to say that we should only accept cleanup/refactoring patches if they are > part of a series of changes that also adds or fixes functionality. > Does anyone else have an opinion on the matter?
I think the exact opposite should be true - accept cleanup and refactoring patches wherever and whenever we can. Heck, I think we should give extra credit for refactoring patches that change no behaviour at all. As to whether all of his changes meet our standards, his points about our standards not being published is a good point, and I think it makes sense to use his patch as a forcing function to publish what bits are not yet decided. > ~Jesse Wolfe > > > On Thu, Nov 4, 2010 at 5:37 AM, Sandor Szuecs <[email protected]> > wrote: > > On Nov 3, 2010, at 9:10 PM, Nigel Kersten wrote: > > >> + # FIXME: dslocal name is fixed. The default should be "Default" if we > >> do not > >> + # want to break existing installations. > >> + DSLOCAL_DIR = "Default" > > > > If this is a pure refactor with no additional functionality, I'd like > > to see some text explaining why you've refactored the way you did, as > > I'm not entirely convinced that this is all worth the effort. > > I refactored the existent code, because I want to do simple feature > changes that other people can follow. I think it is worth to enhance > code quality. I want to undestand parts of code and in this way I do > it, I refactor the code. This is good practice and described in the > "Refactoring" book by Martin Fowler. I try to enhance readabillity of > code that I read, because developers read about 90% of their time code > instead of write code. This was mentioned by Robert C. Martin in the > book "Clean Code". > > Class methods are defined within "class << self .. end" like it is a > widely used and well known ruby. > The extrated constant DSLOCAL_DIR is a simple way to extract the hard > coded MCX dslocal database so you can simply change it in the future > to be a member or config whatever it is used in puppet (I do not know > it right now). > For long methods that could be simple split up, I used extract method. > For example I extracted "raise MCXContentProviderException, 'error > message'" into a method error(msg). I should have written that I > changed the Exception fomr MCXContentProviderException to Puppet::Error, > that was my fault not to mention it earlier. > For more readable code I change constructions like "if ! expression" > to "unless expression", because it is better to read. > I deleted some docs that do not give any more information so these docs > can not "lie" in the future. > I deleted not needed temporary variables, because they are useless and > add no readability in these cases. > > >> + > >> + class << self > >> + # It returns an array of instances of this class. > >> + def instances > >> + mcx_list = [] > >> + TypeMap.keys.each do |ds_type| > >> + ds_path = "/Local/#{DSLOCAL_DIR}/#{TypeMap[ds_type]}" > > All the best, Sandor Szücs > -- > > > > -- > 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. > > > > -- > 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. -- An entrepreneur is somebody who steals office supplies from home and brings them into work. -- Auren Hoffman --------------------------------------------------------------------- Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199 -- 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.
