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? ~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]<puppet-dev%[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.
