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.

Sign up for GitHub

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

Jump to bottom

Fix safe private method calls #142

Closed
wants to merge 1 commit into from
Closed

Fix safe private method calls #142

wants to merge 1 commit into from

Conversation

marshall-lee
Copy link
Member

Related to #140

@marshall-lee
Copy link
Member Author

It should be merged only if #140 is a false alarm. Otherwise it should be fixed differently

@marshall-lee marshall-lee force-pushed the fix_safe_private_method_calls branch from 997b784 to 75b28a7 Compare July 17, 2015 11:29
@marshall-lee marshall-lee force-pushed the fix_safe_private_method_calls branch from 75b28a7 to 603ac30 Compare July 17, 2015 13:25
@dblock
Copy link
Member

dblock commented Jul 17, 2015

So the problem was private methods? I feel like an error that the method is private is almost better than exposing private methods automatically. What do you think?

Either way this needs a CHANGELOG entry, please.

@marshall-lee
Copy link
Member Author

@dblock

It seems like exposing private methods was intented.
Look at the current delegate_attribute implementation

It checks that object responds to attribute name using object.respond_to?(name, true) (not just object.respond_to?(name) and evaluates it by object.send(name) (not by object.public_send(name)).

So this PR just makes safe and unsafe exposures behave identical because now only public methods can exposed when using :safe option.

As for me I don't like exposing private methods. If you think that only public methods should be exposed I could open a separate PR for this too. But it probably can be considered as a breaking change. Is it acceptable here?

@dblock
Copy link
Member

dblock commented Jul 18, 2015

Ok I am with you on this one @marshall-lee. Lets call it a bug and add a line to CHANGELOG please (via --amend) that says something like "Fix: expose works with private methods".

@dblock dblock mentioned this pull request Jul 18, 2015
Closed
@marshall-lee marshall-lee mentioned this pull request Jul 20, 2015
Merged
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Jul 20, 2015
@marshall-lee
Refactoring: extract delegating logic.
e9603fe 
This one solves many problems at once:

 - Fixes ruby-grape#140. For this one I changed specs a bit. It's breaking but I
   think it's worth it.
 - Fixes ruby-grape#142 differently because now `respond_to?(name, true)` stuff is
   incapsulated in `Delegator::PlainObject`.
 - Old `delegate_attribute` catched `NoMethodError` and re-raised it
   with custom message. It's incorrect because `NoMethodError` can occur
deep inside in method call — this exception simply doesn't mean that
attribute doesn't exist.
 - Solves the problem that object is checked to `is_a?(Hash)` and
   `respond_to?(:fetch, true)` multiple times.
 - Extracts delegating logic to separate delegator classes.
 - Removes constructing of `valid_exposures` at all — there's no need in
   this method now.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Jul 20, 2015
@marshall-lee
Refactoring: extract delegating logic.
175ac59 
This one solves many problems at once:

 - Fixes ruby-grape#140. For this one I changed specs a bit. It's breaking but I
   think it's worth it.
 - Fixes ruby-grape#142 differently because now `respond_to?(name, true)` stuff is
   incapsulated in `Delegator::PlainObject`.
 - Old `delegate_attribute` catched `NoMethodError` and re-raised it
   with custom message. It's incorrect because `NoMethodError` can occur
deep inside in method call — this exception simply doesn't mean that
attribute doesn't exist.
 - Solves the problem that object is checked to `is_a?(Hash)` and
   `respond_to?(:fetch, true)` multiple times.
 - Extracts delegating logic to separate delegator classes.
 - Removes constructing of `valid_exposures` at all — there's no need in
   this method now.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Jul 20, 2015
@marshall-lee
Refactoring: extract delegating logic.
1fb3628 
This one solves many problems at once:

 - Fixes ruby-grape#140. For this one I changed specs a bit. It's breaking but I
   think it's worth it.
 - Fixes ruby-grape#142 differently because now `respond_to?(name, true)` stuff is
   incapsulated in `Delegator::PlainObject`.
 - Old `delegate_attribute` catched `NoMethodError` and re-raised it
   with custom message. It's incorrect because `NoMethodError` can occur
deep inside in method call — this exception simply doesn't mean that
attribute doesn't exist.
 - Solves the problem that object is checked to `is_a?(Hash)` and
   `respond_to?(:fetch, true)` multiple times.
 - Extracts delegating logic to separate delegator classes.
 - Removes constructing of `valid_exposures` at all — there's no need in
   this method now.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Jul 20, 2015
@marshall-lee
Refactoring: extract delegating logic.
971d7c4 
This one solves many problems at once:

 - Fixes ruby-grape#140. For this one I changed specs a bit. It's breaking but I
   think it's worth it.
 - Fixes ruby-grape#142 differently because now `respond_to?(name, true)` stuff is
   incapsulated in `Delegator::PlainObject`.
 - Old `delegate_attribute` catched `NoMethodError` and re-raised it
   with custom message. It's incorrect because `NoMethodError` can occur
deep inside in method call — this exception simply doesn't mean that
attribute doesn't exist.
 - Solves the problem that object is checked to `is_a?(Hash)` and
   `respond_to?(:fetch, true)` multiple times.
 - Extracts delegating logic to separate delegator classes.
 - Removes constructing of `valid_exposures` at all — there's no need in
   this method now.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Jul 20, 2015
@marshall-lee
Refactoring: extract delegating logic.
3274bb4 
This one solves many problems at once:

 - Fixes ruby-grape#140. For this one I changed specs a bit. It's breaking but I
   think it's worth it.
 - Fixes ruby-grape#142 differently because now `respond_to?(name, true)` stuff is
   incapsulated in `Delegator::PlainObject`.
 - Old `delegate_attribute` catched `NoMethodError` and re-raised it
   with custom message. It's incorrect because `NoMethodError` can occur
deep inside in method call — this exception simply doesn't mean that
attribute doesn't exist.
 - Solves the problem that object is checked to `is_a?(Hash)` and
   `respond_to?(:fetch, true)` multiple times.
 - Extracts delegating logic to separate delegator classes.
 - Removes constructing of `valid_exposures` at all — there's no need in
   this method now.
@dblock dblock closed this in #147 Jul 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@marshall-lee @dblock