On Sep 13, 2009, at 2:38 PM, Rodrigo Rosenfeld Rosas wrote: > Em 13-09-2009 16:19, Rob Biedenharn escreveu: >> On Sep 13, 2009, at 11:06 AM, Rodrigo Rosenfeld Rosas wrote: >> >>> Hi, I have some doubts considering contributions to Rails. >>> >>> For instance, in attachments follows a patch to change from: >>> >>> default.config = config if config >>> default.config ||= Configuration.new >>> >>> To: >>> >>> default.config = config || Configuration.new >>> >>> That seems more clear to me. >>> >>> This little change really doesn't make much difference but I think >>> it >>> makes the code a bit cleaner... Opening an issue in LightHouse to do >>> just that seems a little too much in my opinion. Is there any way >>> that >>> we can do little changes like this in an easy way, such as happens >>> in >>> docrails project? Maybe some Rails fork for some fast code cleaning >>> with >>> open commit access? >>> >> >> That change is not equivalent unless default.config.nil? to start >> with. > > That is one of the reasons I prefer the last syntax. If one writes: > > default.config = config || Configuration.new > > then, one can be sure that it is not intended that default.config > could > be set prior to Runner#run class method. On the other side, if one > writes: > > default.config ||= config || Configuration.new > > then, it is explicitly that it is ok to set Runner#default.config > before > executing Runner#run.
But, this isn't what the current code does! If there is a value given for the config parameter, then you get: default.config = config regardless of whether default.config already had a value. > > In the current state, we can't be sure if it is expected that one > could > set Runner#default.config before calling Runner#run: > > default.config = config if config > default.config ||= Configuration.new Which is just a specific case of what I'd see as a more general problem. Namely that such "code cleaning" that might superficially appear safe, could actually change the behavior. For well-documented behaviors, this is less likely to happen, of course, but there ought to be some test that clarifies the expectation (and in the process documents the behavior!). At least in the context of a ticket, there is a place to comment on the presence of the existing tests. This kind of alteration in the implicit state expected (i.e., whether there could be a default.config already set before .run was called) is the kind of thing that you'd probably at least want to deprecate first. This is (probably) the last time I'll say something about this. I just want to be sure that people are thinking about the elephant standing in this room. -Rob --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" 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/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
