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. 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 > I'd want to know that such "fast code cleaning" wasn't going to > inadvertently break something. I don't think docrails is blindly merged with master branch, otherwise it would be dangerous. I guess there are some revisions to make sure the merge won't mess with Rails source code. I imagined a similar branch for writing small refactorings that could be fastly reviewed by the core team. I think it would be faster for them to review several small patches at once, instead of a lot of small tickets in LightHouse... Of course it would need more attention to what is paid to docrails, but indeed it would make small changes simpler to handle. > Is there even a test that guarantees > this change doesn't break something? > I don't know. As I stated before, I tried to run the regular_test, but it was broken. I am still waiting for instructions of how to run it... > I think that having to open a ticket is exactly the right hurdle for > the general developer population. With a sufficient history, one might > be offered commit rights that removes this hurdle, but until then I > think that the Rails core team should stand firm on tickets for patches. > I agree for the usual significative changes. But for the cases where small rewrites make the code cleaner without changing Rails behavior, I think opening a ticket for each small improvement that won't affect nothing but code readability would stress the core developers... Rodrigo. --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
