Skip to content
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

delegate to, with private: true option #31944

Merged
merged 1 commit into from Feb 26, 2018
Merged

Conversation

@equivalent
Copy link
Contributor

@equivalent equivalent commented Feb 9, 2018

Summary

alternative version for PR #31933 (pls read description there) with private: true option

class Foo
   # ...
   delegate :foo, :bar, to: :baz
   delegate :car, :dar, to: :daz, private: true
   # ...
end

foo = Foo.new
foo.foo
foo.bar
foo.car # => NoMethodError: private method `car' called for #<Foo:0x000000015e8b10>
foo.dar # => NoMethodError: private method `dar' called for #<Foo:0x000000015e8b10>

#31933 (comment)

@rails-bot
Copy link

@rails-bot rails-bot commented Feb 9, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kamipo (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Feb 9, 2018

Hi @equivalent

Thanks for your PR. Have you discussed it on the mailing list? I haven't found it.

Besides of that, why would you need this? What is the intention?

You can also do the following without adding this PR:

class Foo
   # ...
   delegate :foo, :bar, to: :baz
   # ...
   private
   delegate :car, :dar, to: :daz
   # ...
end
@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 9, 2018

Hi @albertoalmagro

sorry for 2 PRs, I just wanted to create alternative option for this PR #31933 (this is the main one and all my arguments are in it)

I'm sorry but example you provided does not work:

class Bar
   def car
      12
   end
end

class Foo
   private  
   delegate :car, to: :bar  
   def bar
     Bar.new
   end  
end  

Foo.new.car
#=> 12

Foo.new.public_methods - Object.new.public_methods 
#=> [:car]

maybe a security bug ? ... I've mange to get this result on Rails 4.2.5.1, Ruby 2.2.8 and other project Rails 5.1.4, Ruby 2.5.0


I'll copy paste the entire story of PR #31933 in here so the whole context is in place :

Summary

for delegation as public methods there is existing #delegate method :

class Foo
   # ...
   delegate :foo, :bar, to: :baz
   # ...
end

foo = Foo.new
foo.foo
foo.bar

https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/module/delegation.rb#L154

And this PR provides method #delegate_privately

class Foo
   # ...
   delegate_privately :car, :dar, to: :daz
   # ...
end

foo = Foo.new
foo.car  # => NoMethodError: private method `car' called for #<Foo:0x000000015e8b10>
foo.dar # => NoMethodError: private method `car' called for #<Foo:0x000000015e8b10>

Reason

from my understanding, currently the only way how to delegate as private method is by doing:

class Foo
   # ...
   private *delegate( :car, :dar, to: :daz)
   # ...
end

foo = Foo.new
foo.car  # => NoMethodError: private method `car' called for #<Foo:0x000000015e8b10>
foo.dar # => NoMethodError: private method `dar' called for #<Foo:0x000000015e8b10>

and there are some cases when I want to delegate methods to other object and not expose those methods as part of primary object interface methods

Why is private *delegate(...) not enough:

Seems to me like developers might feels discouraged to use the private *delegate(...) as it's 4 responsibility in one line:

  • private
  • splat operator
  • the actual delegation
  • knowledge of what delegate returns

...this way all these 4 responsibilities are hidden in one method #delegate_privately and tested outside of the application => Rails framework

Also there is misconception amongs developers that this works (which does not)

class Bar
   def car
      12
   end
end

class Foo
   private  
   delegate :car, to: :bar  
   def bar
     Bar.new
   end  
end  

Foo.new.car
#=> 12

Foo.new.public_methods - Object.new.public_methods 
#=> [:car]

Actually that is my case, I was (at least) 4 years in understanding that this works, and never bothered to check it 😞

Related discussion:

@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 9, 2018

so whether you guys like solution PR #31944 (this one):

class Foo
   # ...
   delegate :foo, :bar, to: :baz
   delegate :car, :dar, to: :daz, private: true
   # ...
end

or you guys like solution in PR #31933 :

class Foo
   # ...
   delegate :foo, :bar, to: :baz
   delegate_privately :car, :dar, to: :daz
   # ...
end

it's up to you which one you pick and close the other. Both are doing the same and I like both solutions

Point is this does not work :

class Foo
  # ...
   delegate :foo, :bar, to: :baz
   private
   delegate :car, :dar, to: :daz
  # ...
end

And this works but it's too many responsibilities(feels like there should be a easier way) and stuff may go wrong and not many dev. know about this:

class Foo
  # ...
   delegate :foo, :bar, to: :baz
   private *delegate(:car, :dar, to: :daz)
  #...
end
@matthewd
Copy link
Member

@matthewd matthewd commented Feb 9, 2018

Have you discussed it on the mailing list? I haven't found it.

@albertoalmagro where did you get the impression that people should do that? Do we need to fix some documentation? The mailing list is the least-bad option if someone wants to discuss something they haven't yet implemented, but "here's a diff, should I open a PR?" posts are annoyingly pointless.

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Feb 9, 2018

@matthewd in the Rails contributing guide, as this would be a new feature in my opinion. http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#what-about-feature-requests-questionmark

And also here: #31932 (comment)

@matthewd
Copy link
Member

@matthewd matthewd commented Feb 9, 2018

If you'd like feedback on an idea for a feature before doing the work to make a patch

feature proposals without code

Any suggestions on how those could be phrased differently?

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Feb 9, 2018

OK, sorry, all yours.

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Feb 9, 2018

Sorry for the noise @equivalent it seems I need some holidays

@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 9, 2018

@albertoalmagro no problem at all mate :)

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 12, 2018

Implementation looks good to me, but I'm not sure I would use this syntax sugar so I'll defer to @dhh.

@dhh
Copy link
Member

@dhh dhh commented Feb 12, 2018

I like this 👍

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 12, 2018

The tests are broken, can you take a look?

@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 13, 2018

@rafaelfranca sorry didn't have time over weekend, will fix them now;

activesupport/lib/active_support/core_ext/module/delegation.rb Outdated
@@ -213,6 +231,9 @@ def delegate(*methods, to: nil, prefix: nil, allow_nil: nil)

module_eval(method_def, file, line)
end

method_names.each { |n| module_eval("private #{n}", file, line) } if private

This comment has been minimized.

@kamipo

kamipo Feb 13, 2018
Member

Should be "private :#{n}" or "private #{n.inspect}".

This comment has been minimized.

@matthewd

matthewd Feb 13, 2018
Member

Maybe just private(*method_names) if private?

This comment has been minimized.

@equivalent

equivalent Feb 13, 2018
Author Contributor

@kamipo 🤔

actually I've discovered that that will not work either

ModuleTest#test_some_public_some_private_delegate_with_private_option:
SyntaxError: test/core_ext/module_test.rb:469: syntax error, unexpected ':', expecting end-of-input
private :city

but this does:

 method_names.each { |n| self.send(:private, n) } if private

Do you think that's ok ?

It's seems that private when used in private :argument is method send on a current object of the class ( that means current Module/Class) and module_eval "private" will not work

This comment has been minimized.

@kamipo

kamipo Feb 13, 2018
Member

The private method can be passed method names.
So it just can be simplified as send(:private, *method_names) if private.

This comment has been minimized.

@equivalent

equivalent Feb 13, 2018
Author Contributor

I've squashed the commits to single commit, just so you know, only bit that changed is 4167193#diff-01f2db2585c06eca03cf014cfe51c50eR235

This comment has been minimized.

@equivalent

equivalent Feb 13, 2018
Author Contributor

@kamipo ah, ok sorry I didn't see this, I'm on it

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 13, 2018

Should we also support protected as well? Alternatively there's delegate :x, to: :@y, visibility: :private

I know we've established that the code below doesn't work. But does anyone here know why we can't?

private
  delegate :x, to: :@y

The API feels partially weird once users have to do the below. Though the private keyword is weird in general because it'd only apply to the current self — it doesn't catch constants and class methods iirc.

private
  delegate :x, to: :@y, private: true
@matthewd
Copy link
Member

@matthewd matthewd commented Feb 13, 2018

I know we've established that the code below doesn't work. But does anyone here know why we can't?

Yeah.. private is lexically scoped, and MRI doesn't expose that information to Ruby-land.

I don't think protected is common enough to bother with, nor to justify a more verbose visibility: option. (Personally I'm dubious that private: would get enough use too, but it does seem comparatively plausible.)

@equivalent equivalent force-pushed the equivalent:delegate_private branch Feb 13, 2018
@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 13, 2018

@matthewd it really depends whether developer needs to implement Interface Segregation Principle (in sense that expose only methods you want to be callable). I agree with protected not having much value but private is really valuable.

From top of my mind, if you are writing test where you compare 2 objects interfaces expects( Foo.new.public_methods(false)).to match_array(Bar.new.public_methods(false))

@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 13, 2018

@kaspth @matthewd let me know if you want the protected as well, I can add that in. But I do agree with @matthewd that private is good enough. At least for first iteration of this PR.

...also I want to point out that commits are now squashed #31944 (comment) original implementation changed to this 4167193#diff-01f2db2585c06eca03cf014cfe51c50eR235

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 13, 2018

Totally fine with just private: true 👍

Would @kamipo's send(:private, *method_names) if private work though?

@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 13, 2018

@kaspth ah, sorry I've squashed the commits, original history is lost

my (failing) original was:

    method_names.each { |n|   module_eval("private #{n}", file, line) } if private

that is evaluated as 'private city'

and although it was working when I've tested it with rails c inside a Rails 5 project that used Gemfile 'rails', path: '/home/equivalent/my_clone/rails the Rails core tests were failing

kamipo was proposing (if I understood it correctly ):

    method_names.each { |n|   module_eval("private #{n.inspect}", file, line) } if private

so that evaluated string was 'private :city'

UPDATE, actually @kamipo was proposing much better solution send(:private, *method_names) if private I just didn't see the latest comment
#31944 (comment) sorry 😿

That was failing too

The current implementation (that seems to pass, and work)

    method_names.each { |n| self.send(:private, n) } if private

this is sending a message to Class method / Module method object to "privatize" the method :city for that class/module

activesupport/test/core_ext/module_test.rb Outdated
@place = place
end

private(*delegate(:street, :city, to: :@place, private: true))

This comment has been minimized.

@kamipo

kamipo Feb 13, 2018
Member

What is this testing?

This comment has been minimized.

@equivalent

equivalent Feb 13, 2018
Author Contributor

as lot of developers were used to writing the private delegated methods this way

private *delegate(:city, to: @profile)

e.g.: https://stackoverflow.com/a/35858112/473040

I wanted to ensure that nothing will break when they decide to change it to

private *delegate(:city, to: :@profile, private: true)

I know that this is enough onece thin PR is there:

delegate(:city, to: :@profile, private: true)

I just wanted to be sure this will not break

private *delegate(:city, to: :@profile, private: true)

I can remove that test if you want, but I've found it valuable (As I've manage to write my first implementation of this PR that was accidentally disabling the splat private format)

This comment has been minimized.

@matthewd

matthewd Feb 13, 2018
Member

I don't think we care about private *delegate(.., private: true) (double-private), but a test for private *delegate(..) seems reasonable.

(Though a test that directly checks the return value would also be sufficient.)

This comment has been minimized.

@equivalent

equivalent Feb 13, 2018
Author Contributor

should I refactor that test to check the return value then ? ...or just remove ?

This comment has been minimized.

@equivalent

equivalent Feb 13, 2018
Author Contributor

ok how about this?: 8198427

@kamipo
Copy link
Member

@kamipo kamipo commented Feb 13, 2018

My latest suggestion (#31944 (comment)) is like here:

diff --git a/activesupport/lib/active_support/core_ext/module/delegation.rb b/activesupport/lib/active_support/core_ext/module/delegation.rb
index f3d4db667c..80955900c4 100644
--- a/activesupport/lib/active_support/core_ext/module/delegation.rb
+++ b/activesupport/lib/active_support/core_ext/module/delegation.rb
@@ -232,7 +232,7 @@ def delegate(*methods, to: nil, prefix: nil, allow_nil: nil, private: nil)
       module_eval(method_def, file, line)
     end
 
-    method_names.each { |n| self.send(:private, n) } if private
+    send(:private, *method_names) if private
     method_names
   end
 

I confirmed that it has passed.

@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 13, 2018

@kamipo sorry didn't seen your latest comment, thank you very much that's much better approach. Now implemented.

should I squash the commits ?

@matthewd
Copy link
Member

@matthewd matthewd commented Feb 13, 2018

No need for send either: #31944 (comment)

private(*method_names) if private
@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 13, 2018

all suggestions implemented

Gem av; ujs is faling on build (everything else is passing ) is that caused by me ? or is this a known issue ?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 17, 2018

Can you add a CHANGELOG entry and squash your commits?

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 18, 2018

@equivalent really like how this turned out, great work! 👏 Now all that's left to cross the finish line is @rafaelfranca's bits in #31944 (comment) 😊

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Feb 19, 2018

Great job @equivalent ! I also was one of the confused developers who thought that putting this under private would solve the issue. Thanks a lot for the PR and for teaching me this! 👏

@equivalent equivalent force-pushed the equivalent:delegate_private branch 2 times, most recently Feb 20, 2018
@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 20, 2018

@rafaelfranca

ok I've added Changelog (let me know if it's too long I can make it shorter, also I'm referencing this issue number in the Changelog as there may be developers investigating why not to do just private \n delegate )

@kamipo @matthewd @kaspth @albertoalmagro @rafaelfranca @dhh
Thank you all for all the help, ideas and patience before we got this right. 🎊

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 24, 2018

@equivalent no problem! We'll need a rebase for the changelog, sorry! Then I'll merge.

@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 26, 2018

@kaspth ok done the rebase.

@simi thx for helping me with the rebase (...I've manage to f.up initilal rebase badly due to bad .git/config in the fork stored on my laptop)

@equivalent equivalent force-pushed the equivalent:delegate_private branch Feb 26, 2018
@kaspth
Copy link
Member

@kaspth kaspth commented Feb 26, 2018

@equivalent looks like there's still some leftover time zone changes from the rebase :)

@equivalent equivalent force-pushed the equivalent:delegate_private branch to 06f0675 Feb 26, 2018
@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 26, 2018

@kaspth ok, try now

@kaspth kaspth merged commit 9cf7e79 into rails:master Feb 26, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codeclimate All good!
Details
@kaspth
Copy link
Member

@kaspth kaspth commented Feb 26, 2018

Verified locally on Ruby 2.4. Welcome, @equivalent! You're now a contributor to Rails 👏

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Feb 26, 2018
Previously it was removed by rails#32106 since it was backported to `5-2-stable`.

[ci skip]
rafaelfranca added a commit that referenced this pull request Feb 26, 2018
…ed-by-31944

Remove extra changelog added by #31944
@equivalent
Copy link
Contributor Author

@equivalent equivalent commented Feb 26, 2018

yes !

@equivalent equivalent deleted the equivalent:delegate_private branch Feb 26, 2018
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Feb 28, 2018
…turns correct value

Remove extra comments `# Asking for private method` in activesupport/test/core_ext/module_test.rb
Improve docs of using `delegate` with `:private`
Update changelog of rails#31944
@meinac
Copy link
Contributor

@meinac meinac commented Mar 21, 2018

This was a common issue which I used see in my team. Thanks for adding the functionality to make the delegated methods private without dealing with the abomination private(*delegate(...)).
Implementing the protected feature does not add too much complexity therefore I suggest implementing it as well, even though some of you think it's not widely used. At least the reason to add it could be; since the private option is implemented, one can think the protected is implemented as well. To prevent such astonishment, the protected option should be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.