Doubts about contributions

rubyonrails-core
1

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?

Then, here is the next question. I was reading this guide:

But it was not clear if I should run all tests for all Rails modules when changing something like this. First, I didn't see an option to run the tests only for sqlite3, for instance in Rails root, just when testing ActiveRecord alone. Anyway, I tried to:

> cd railties > rake regular_test (I didn't find any other related task)

First, it asked to install the rack-test gem (maybe we should state that in Rails guides, anyway). Then it seems that the testing is requiring Initilizer twice for some reason:

rails/railties/lib/initializer.rb:17: It looks like initializer.rb was required twice (RuntimeError)

How should I run the Rails tests and which gems I need to install and what configurations should I do?

Thanks in advance,

Rodrigo.

0001-Code-cleaning.patch (789 Bytes)

2

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. I'd want to know that such "fast code cleaning" wasn't going to inadvertently break something. Is there even a test that guarantees this change doesn't break something?

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.

-Rob

3

   

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.

4

Rails has a well-established policy with several levels of gatekeeping for getting code into the project. From my point of view, this is one of the reasons for the generally high quality of the code: we can be assured that every code change is reviewed and approved by at least one core committer who is intimately familiar with Rails. I for one would argue strenuously against loosening this policy. Given that there have been over 1400 contributors to Rails so far, I think we have pretty good evidence that this policy is not too restrictive or onerous.

If you think your small changes are real wins for Rails, I urge you to follow the established procedures: create a patch, open a Lighthouse ticket, lobby for support. If other developers agree with you, your changes will have smooth sailing and you'll be able to bask in being a core contributor. But part of what we depend on is the established principle of "many eyes make all bugs shallow." No one, not even core members, just changes the source code without a sanity check.

If you're having trouble getting the tests to run, there are several resources you can refer to:

- The "Contributing to Rails" Guide rails.info - The Pre-flight Checklist put together by RailsBridge http://wiki.railsbridge.org/projects/railsbridge/wiki/Pre-flight_Checklist - The setup notes for the Rails CI server http://github.com/rails/rails/blob/e033b5d037c303a34e0c5aec2b38ec6270f00f86/ci/ci_setup_notes.txt - The Contributing to Rails Railscast #113 Contributing to Rails with Git - RailsCasts

If you're still stuck after working through these various guides, you could try asking specific questions here or in #rails-contrib on IRC. Or you could join us at the next BugMash (September 26) when experienced Rails developers and core committers will be available to help get you over the initial hurdles towards contributing.

Mike

5

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...       Rails has a well-established policy with several levels of gatekeeping for getting code into the project. From my point of view, this is one of the reasons for the generally high quality of the code: we can be assured that every code change is reviewed and approved by at least one core committer who is intimately familiar with Rails.

I am not suggesting this stops being done. My suggestion is that for small patches, there was an open repository so that the core team could review a lot of them at once, before applying. For instance, if one knows deeply about ActionPack, he could take a look at all changes in ActionPack at once and decide which commits to accept. Anyway, that was just a suggestion.

I for one would argue strenuously against loosening this policy. Given that there have been over 1400 contributors to Rails so far, I think we have pretty good evidence that this policy is not too restrictive or onerous.     I think this works great for new features or bug correction patches. But for small changes aiming at code cleaning or correcting method or variable mispellings, this would be too much in my opinion.

If you think your small changes are real wins for Rails,

That is the question. I don't know what you mean by "real win". It is too subjective. I give very much importance about code readability, even when the changes are so small... But most won't care about it. Opening a patch to make Rails a little bit more readable would probably make the reviewers angry in the sense that it would be wasting their times. On the other side, if there is some branch aimed to do just that, then one could take a look from time to time in all these little changes at once. Maybe a lot of little improvements could make a "real win", I don't know...

I urge you to follow the established procedures: create a patch, open a Lighthouse ticket, lobby for support.

That is what I intend to do for supporting a new :full_messages parameter in validations. But I am not talking about this kind of patch. I am just talking about small changes patches that won't affect the end user, but only Rails code maintainance.

If other developers agree with you, your changes will have smooth sailing and you'll be able to bask in being a core contributor. But part of what we depend on is the established principle of "many eyes make all bugs shallow." No one, not even core members, just changes the source code without a sanity check.     Thanks God! :slight_smile:

If you're having trouble getting the tests to run, there are several resources you can refer to:

- The "Contributing to Rails" Guide rails.info     I've already done that as stated in the first message of the topic. But it seems a lot incomplete. There are no references for installing mocha or other dependent gems. It is not explained how to speed up tests. For instance, the kind of patch I mentioned should potentially only affect the railties module. There is no such "rake test" for railties. There is a 'regular_test' task, but it doesn't run because initializer.rb is being required twice for some reason.

I also tried following the activerecord tests, but it also fails:

./test/cases/../../lib/active_record/base.rb:1984:in `method_missing': undefined method `silence_warnings' for ActiveRecord::Base:Class (NoMethodError)

- The Pre-flight Checklist put together by RailsBridge Create new page · railsbridge/docs Wiki · GitHub - The setup notes for the Rails CI server http://github.com/rails/rails/blob/e033b5d037c303a34e0c5aec2b38ec6270f00f86/ci/ci_setup_notes.txt     I've read these links and the only news I found about testing was about the need of the mocha gem (which mention is lacking in the Rails Guides). - The Contributing to Rails Railscast #113 Contributing to Rails with Git - RailsCasts     This was probably the first screencast I watched about this subject, a lot of time ago.

If you're still stuck after working through these various guides, you could try asking specific questions here or in #rails-contrib on IRC.     So, here you are. Is it just me or someone else is getting trouble running the Rails tests on master branch? I can not run regular_test in railties nor the tests in activerecord (the two that I tried). This is the message from regular_test:

rails/railties/lib/initializer.rb:17: It looks like initializer.rb was required twice (RuntimeError)

Or you could join us at the next BugMash (September 26) when experienced Rails developers and core committers will be available to help get you over the initial hurdles towards contributing.     I would like to but the BugMash is target only to bugs. I'd prefer to work in the :full_messages patch if it is possible.

Thanks for your help,

Rodrigo.

6

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

7

ld argue strenuously against loosening this policy. Given that there have been over 1400 contributors to Rails so far, I think we have pretty good evidence that this policy is not too restrictive or onerous.

I think this works great for new features or bug correction patches. But for small changes aiming at code cleaning or correcting method or variable mispellings, this would be too much in my opinion.

You'd be wrong, frankly. If you dig through the Rails commit history, you'll find patches as small as one character. ...

Or you could join us at the next BugMash (September 26) when experienced Rails developers and core committers will be available to help get you over the initial hurdles towards contributing.

I would like to but the BugMash is target only to bugs. I'd prefer to work in the :full_messages patch if it is possible.

This is also wrong. The BugMash targets cleaning up the patch queue in general.

Mike