Prevent creation of instance methods when `instance_reader = false`, Grammar checks, Conditional statements combined #13401

Merged
merged 1 commit into from Dec 19, 2013

Conversation

Projects
None yet
3 participants
Contributor

akshay-vishnoi commented Dec 19, 2013

No description provided.

- # Generate the public methods name, name=, and name?
+ # Generate the public methods name, name=, and name?.
@dmathieu

dmathieu Dec 19, 2013

Contributor

An interrogation point is a point. You don't need one after that.

@akshay-vishnoi

akshay-vishnoi Dec 19, 2013

Contributor

It is not an interrogation statement and ? belongs to method name?.

@@ -42,7 +42,7 @@ def test_simple_accessor_declaration_with_instance_reader_false
single_class.superclass_delegating_accessor :no_instance_reader, :instance_reader => false
assert_respond_to single_class, :no_instance_reader
assert_respond_to single_class, :no_instance_reader=
- assert !single_class.public_instance_methods.map(&:to_s).include?("no_instance_reader")
+ assert (single_class.public_instance_methods & [:no_instance_reader, :no_instance_reader?, :_no_instance_reader]).empty?
@dmathieu

dmathieu Dec 19, 2013

Contributor

What is the point of this? It decreases code readability.

@akshay-vishnoi

akshay-vishnoi Dec 19, 2013

Contributor

Actually I wanted to ensure none of the listed methods should be available to single_class instances. In previous case, test case was checking only for no_instance_reader, but it should check for all.

@akshay-vishnoi

akshay-vishnoi Dec 19, 2013

Contributor

If I go with the previous case then I will have to write assert statement 3 times, given that map(&:to_s) will be called for all of them. Should I stick with previous version?

@rafaelfranca

rafaelfranca Dec 19, 2013

Owner

You can store the instance methods in a local variable and do three assertions.

@akshay-vishnoi

akshay-vishnoi Dec 19, 2013

Contributor

Yes I can do this. Thanks

+ # If an instance_reader is needed, generate public instance methods name and name?.
+ if options[:instance_reader] != false
+ define_method(name) { send("_#{name}") }
+ define_method("#{name}?") { send("#{name}").present? }
@dmathieu

dmathieu Dec 19, 2013

Contributor

👍

@rafaelfranca

rafaelfranca Dec 19, 2013

Owner

This is cosmetic change so it should not be included in this commit

@akshay-vishnoi

akshay-vishnoi Dec 19, 2013

Contributor

@rafaelfranca But you had asked to change these kind of changes in Issues. Here same condition is used two times.

@rafaelfranca

rafaelfranca Dec 19, 2013

Owner

ok, but remove the present?

singleton_class.send(:define_method, name) { send("_#{name}") }
- singleton_class.send(:define_method, "#{name}?") { !!send("_#{name}") }
+ singleton_class.send(:define_method, "#{name}?") { send("_#{name}").present? }
@rafaelfranca

rafaelfranca Dec 19, 2013

Owner

Please don't use present? here. There is no need for this change

@akshay-vishnoi

akshay-vishnoi Dec 19, 2013

Contributor

@rafaelfranca But it looks more readable. Am I missing something?

@rafaelfranca

rafaelfranca Dec 19, 2013

Owner

Yes, you are adding an unnecessary dependency for this file.

@akshay-vishnoi

akshay-vishnoi Dec 19, 2013

Contributor

Okay I will remove it.

Prevent creation of instance methods when `instance_reader = false`, …
…Grammar checks, Conditional statements combined
Contributor

akshay-vishnoi commented Dec 19, 2013

@rafaelfranca Updated. Please have look at it now

rafaelfranca added a commit that referenced this pull request Dec 19, 2013

Merge pull request #13401 from akshay-vishnoi/refactor
Prevent creation of instance methods when `instance_reader = false`, Grammar checks, Conditional statements combined

@rafaelfranca rafaelfranca merged commit 21fe17a into rails:master Dec 19, 2013

1 check passed

default The Travis CI build passed
Details
Owner

rafaelfranca commented Dec 19, 2013

Thank you

Contributor

akshay-vishnoi commented Dec 19, 2013

Thanks @rafaelfranca ... 💚 💛 💙

@akshay-vishnoi akshay-vishnoi deleted the akshay-vishnoi:refactor branch Dec 19, 2013

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