refactor reset_primary_key and change !blank? to present? in get_primary... #8800

Merged
merged 1 commit into from Jan 8, 2013

Conversation

Projects
None yet
7 participants
@acapilleri
Contributor

acapilleri commented Jan 7, 2013

..._key

@steveklabnik

View changes

activerecord/lib/active_record/attribute_methods/primary_key.rb
@@ -68,15 +68,15 @@ def quoted_primary_key
end
def reset_primary_key #:nodoc:
- if self == base_class
- self.primary_key = get_primary_key(base_class.name)
+ self.primary_key = if self == base_class

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Jan 7, 2013

Member

Ugh. I understand the want to get rid of duplication, but I think I'm 👎 on this.

@steveklabnik

steveklabnik Jan 7, 2013

Member

Ugh. I understand the want to get rid of duplication, but I think I'm 👎 on this.

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Jan 7, 2013

Contributor

@steveklabnik thanks for review, in this case i think that the readably of the code is a little better.

@acapilleri

acapilleri Jan 7, 2013

Contributor

@steveklabnik thanks for review, in this case i think that the readably of the code is a little better.

This comment has been minimized.

Show comment Hide comment
@davidcelis

davidcelis Jan 7, 2013

Contributor

I disagree. I've always found attribute = if condition statements to be less readable at a glance. I think this is less readable.

@davidcelis

davidcelis Jan 7, 2013

Contributor

I disagree. I've always found attribute = if condition statements to be less readable at a glance. I think this is less readable.

This comment has been minimized.

Show comment Hide comment
@PikachuEXE

PikachuEXE Jan 8, 2013

Contributor

I agree with @steveklabnik
Although this removes the duplication but makes it harder to read

And also when there are more things to add on a specific condition
The form before this refactor is easier to change

@PikachuEXE

PikachuEXE Jan 8, 2013

Contributor

I agree with @steveklabnik
Although this removes the duplication but makes it harder to read

And also when there are more things to add on a specific condition
The form before this refactor is easier to change

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Jan 8, 2013

Owner

🚲🏡

@jeremy

jeremy Jan 8, 2013

Owner

🚲🏡

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Jan 8, 2013

Contributor

I like attribute= if, in this case we have just less 5 line of code with a very simple logic, anyway my refactoring was focused on the get_primary_key method, @guilleiguaran what do you think about? let' me if I have to rollback reset_primary_keyfor merging. Thanks in advance

@acapilleri

acapilleri Jan 8, 2013

Contributor

I like attribute= if, in this case we have just less 5 line of code with a very simple logic, anyway my refactoring was focused on the get_primary_key method, @guilleiguaran what do you think about? let' me if I have to rollback reset_primary_keyfor merging. Thanks in advance

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Jan 8, 2013

Owner

Agree with @jeremy here - this is a subjective change and I don't think it's worth polluting the git history for it. The simplification of get_primary_key is is marginally worthwhile so if you update the PR as I suggested below and remove this change I think we can merge it.

@pixeltrix

pixeltrix Jan 8, 2013

Owner

Agree with @jeremy here - this is a subjective change and I don't think it's worth polluting the git history for it. The simplification of get_primary_key is is marginally worthwhile so if you update the PR as I suggested below and remove this change I think we can merge it.

This comment has been minimized.

Show comment Hide comment
@clemens

clemens Jan 8, 2013

Contributor

How about a ternary?

self.primary_key = self == base_class ? get_primary_key(base_class.name) : base_class.primary_key

I see the point about not polluting the git history, though, as it has no real world benefit. ;)

@clemens

clemens Jan 8, 2013

Contributor

How about a ternary?

self.primary_key = self == base_class ? get_primary_key(base_class.name) : base_class.primary_key

I see the point about not polluting the git history, though, as it has no real world benefit. ;)

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Jan 8, 2013

Contributor

cc / @pixeltrix

Contributor

acapilleri commented Jan 8, 2013

cc / @pixeltrix

@pixeltrix

View changes

activerecord/lib/active_record/attribute_methods/primary_key.rb
end
end
def get_primary_key(base_name) #:nodoc:
- return 'id' unless base_name && !base_name.blank?
+ return 'id' unless base_name.present?

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Jan 8, 2013

Owner

Couldn't this be return 'id' if base_name.blank? since both nil and "" return true for blank?

@pixeltrix

pixeltrix Jan 8, 2013

Owner

Couldn't this be return 'id' if base_name.blank? since both nil and "" return true for blank?

This comment has been minimized.

Show comment Hide comment
@PikachuEXE

PikachuEXE Jan 8, 2013

Contributor
@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Jan 8, 2013

Contributor

@pixeltrix done, thanks for your detailed review

Contributor

acapilleri commented Jan 8, 2013

@pixeltrix done, thanks for your detailed review

pixeltrix added a commit that referenced this pull request Jan 8, 2013

Merge pull request #8800 from acapilleri/primary_key
refactor reset_primary_key and change !blank? to present? in get_primary...

@pixeltrix pixeltrix merged commit de21da5 into rails:master Jan 8, 2013

@acapilleri acapilleri deleted the acapilleri:primary_key branch Jan 8, 2013

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