/rails

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove over meta programming in AR::Relation #26182

Merged
merged 1 commit into from Aug 24, 2016
+59 −94

Conversation

Reviewers
No reviews
Assignees

@matthewd matthewd

Labels
Projects
None yet
Milestone
No milestone
6 participants
@bogdan @rails-bot @rafaelfranca @matthewd @kamipo @vipulnsward
@bogdan
Contributor

bogdan commented Aug 16, 2016

Summary

Introduced low level methods #set and #get for setting query attributes:

relation.set(:where, {id: 1})
relation.get(:includes)

Used those internally when working with relation's attributes
at the abstract level
@rails-bot

This comment has been minimized.

Sign in to view

rails-bot commented Aug 16, 2016

r? @matthewd

(@rails-bot has picked a reviewer for you, use r? to override)

@rails-bot rails-bot assigned matthewd Aug 16, 2016

@vipulnsward vipulnsward added the activerecord label Aug 16, 2016

@rafaelfranca

This comment has been minimized.

Sign in to view
Member

rafaelfranca commented Aug 16, 2016

There are some style violations in the PR. The implementation is good to me, could you fix the violations?

@bogdan bogdan force-pushed the bogdan:remove-relation-metaprogramming branch Aug 16, 2016

@matthewd

This comment has been minimized.

Sign in to view
Member

matthewd commented Aug 16, 2016

I don't really like the names get and set, but I'm struggling to offer better suggestions. We should at least consider prepending an underscore -- relation is a namespace we share with user methods, so short + generic + undifferentiated seems conflict-prone.
@bogdan

This comment has been minimized.

Sign in to view
Contributor

bogdan commented Aug 17, 2016

@matthewd from the other hand I sometimes would want to use get and set in the rails app instead of not so convenient and strange accessor (especially get). So I want to treat them as official Relation API.
@rafaelfranca

This comment has been minimized.

Sign in to view
Member

rafaelfranca commented Aug 17, 2016 •
edited

Well, it will be private API so it can't be official Relation API.
@rafaelfranca

This comment has been minimized.

Sign in to view
Member

rafaelfranca commented Aug 17, 2016

get_property/set_property maybe?
@bogdan

This comment has been minimized.

Sign in to view
Contributor

bogdan commented Aug 17, 2016

Ok, I'll gather all variants for you:

  • _get
  • __get__
  • get
  • get_property
  • []=
  • get_value
  • relation_value=

Choose wisely.

@bogdan bogdan force-pushed the bogdan:remove-relation-metaprogramming branch 2 times, most recently Aug 17, 2016

@bogdan

This comment has been minimized.

Sign in to view
Contributor

bogdan commented Aug 18, 2016

@rafaelfranca changed to set/get_value as they seem the most relevant to current terminology of "values" inside relation.
@kamipo

kamipo reviewed Aug 18, 2016
View changes

activerecord/lib/active_record/relation/query_methods.rb Outdated
def #{name}_value # def readonly_value
@values[:#{name}] # @values[:readonly]
def #{method_name} # def includes_values
get_value(#{name.inspect}) # get_value(:includes)

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo Aug 18, 2016

Member

Broken an alignment of comments.
@kamipo

kamipo reviewed Aug 18, 2016
View changes

activerecord/lib/active_record/relation/query_methods.rb Outdated
assert_mutability! # assert_mutability!
@values[:#{name}] = value # @values[:readonly] = value
def #{method_name}=(value) # def includes_values=(value)
set_value(#{name.inspect}, value) # set_value(:includes, value)

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo Aug 18, 2016

Member

ditto.

@bogdan bogdan force-pushed the bogdan:remove-relation-metaprogramming branch Aug 18, 2016

@kamipo

kamipo reviewed Aug 18, 2016
View changes

activerecord/lib/active_record/relation/query_methods.rb Outdated
end
# Sets the relation value with the given name
def set_value(name, value) #:nodoc:

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo Aug 18, 2016

Member

I am a little curious about #:nodoc:. get_value is # :nodoc: (including a space).

@bogdan bogdan force-pushed the bogdan:remove-relation-metaprogramming branch Aug 18, 2016

@kamipo

This comment has been minimized.

Sign in to view
Member

kamipo commented Aug 18, 2016

another variants: read_value/write_value.
activerecord/lib/active_record/relation/query_methods.rb
if !VALID_UNSCOPING_VALUES.include?(scope)
raise ArgumentError, "Called unscope() with invalid unscoping argument ':#{scope}'. Valid arguments are :#{VALID_UNSCOPING_VALUES.to_a.join(", :")}."
end
set_value(scope, nil)

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo Aug 18, 2016

Member

Is setting nil safe for MULTI_VALUE_METHODS?

This comment has been minimized.

Sign in to view
Copy link
@bogdan

bogdan Aug 18, 2016 •
edited

Contributor

It results in overwriting by default in get on line 929

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo Aug 18, 2016

Member

I see. Thanks for your explanation!
@bogdan

This comment has been minimized.

Sign in to view
Contributor

bogdan commented Aug 23, 2016

@rafaelfranca @matthewd ready to merge?
@bogdan
Remove over meta programming in AR::Relation 
Introduced low level methods #set_value and #get_value for setting query attributes:

  relation.set_value(:where, {id: 1})
  relation.get_value(:includes)

Used those internally when working with relation's attributes
at the abstract level
Loading status checks…
5b42628

@bogdan bogdan force-pushed the bogdan:remove-relation-metaprogramming branch to 5b42628 Aug 23, 2016

@rafaelfranca rafaelfranca merged commit 8c83a44 into rails:master Aug 24, 2016
2 checks passed

2 checks passed

codeclimate Code Climate has skipped analysis of this commit.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

kamipo added a commit that referenced this pull request Jan 4, 2018

@kamipo
Partial revert the changing default value of `readonly_value` 
This is a partial revert of #26182. There is no reason to change the
default value.
Loading status checks…
7eaae93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment