Enhance readability of NestedAttributes and its test suite #14339

Closed
wants to merge 1 commit into
from

3 participants

@m-Peter

The following assignment and condition checking in one go does not
communicate its intent immediately:

if reflection = reflect_on_association(association_name)

and thus it is splitted.

Some tests state that a new record should not be built when the
:reject_if proc evaluates to false and vice versa. That's not the
case though, because a new record will be rejected if the respective
proc evaluates to true.

Besides its main purpose, a test suite can provide a general outline
of what to expect when we look directly at the implementation so
I've added a few tests to help achieve this.

Peter Markou Enhance readability of NestedAttributes and its test suite
The following assignment and condition checking in one go does not
communicate its intent immediately:

    if reflection = reflect_on_association(association_name)

and thus it is splitted.

Some tests state that a new record should not be built when the
:reject_if proc evaluates to false and vice versa. That's not the
case though, because a new record will be rejected if the respective
proc evaluates to true.

Besides its main purpose, a test suite can provide a general outline
of what to expect when we look directly at the implementation so
I've added a few tests to help achieve this.
f6e4ff7
@kennym kennym commented on the diff Mar 10, 2014
activerecord/lib/active_record/nested_attributes.rb
@@ -305,7 +305,8 @@ def accepts_nested_attributes_for(*attr_names)
options[:reject_if] = REJECT_ALL_BLANK_PROC if options[:reject_if] == :all_blank
attr_names.each do |association_name|
- if reflection = reflect_on_association(association_name)
@kennym
kennym added a line comment Mar 10, 2014

IMHO this is more readable than breaking it up into two lines.

@dmathieu
dmathieu added a line comment Mar 16, 2014

👎 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmathieu

I'm going to close this, as the change is cosmetic only.
I invite you to read the contribute to ruby on rails guide.

Changes that are cosmetic in nature and do not add anything substantial to the stability, functionality, or testability of Rails will generally not be accepted.

Also, I don't think splitting this line in two improves readability.

@dmathieu dmathieu closed this Mar 16, 2014
@m-Peter

Okay then, I guess it's only me who believes that usage of = in if statements is not a good practice :(. The commit though has two files changed, and the changes regarding the test suite fix some typos and add some tests. Do you consider them cosmetic too?

@thedarkone

Although I agree with @dmathieu and @kennym about the if foo = bar change, the rest of the pull request has been IMHO rejected prematurely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment