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

Test that you can reopen nested exposures #63

Merged
merged 2 commits into from
Mar 12, 2014

Conversation

Nerian
Copy link
Contributor

@Nerian Nerian commented Mar 6, 2014

Related to #62

The test was actually not right. The exposures inside the :user exposure in the root entity Person are getting overwritten in the children.

class Person < Grape::Entity
##This gets overwritten by the exposure in Student. So there is no `:in_first` exposure.
  expose :user do
    expose(:in_first) { |_| 'value' }
  end
end

class Student < Person
  expose :user do
    expose(:user_id) { |_| 'value' }
    expose(:user_display_id, as: :display_id) { |_| 'value' }
  end
end

I corrected the test to reproduce the issue.

@AMar4enko
Copy link
Contributor

So, this is expected behavior, then nested exposure in subclass merged with parent class?

@dblock
Copy link
Member

dblock commented Mar 7, 2014

Looks suspicious. Why would exposures merge with the exposure of a parent class? If you're going to override a method in a class hierarchy, you'd expect that the parent method is not called unless you do something like super, no?

@Nerian
Copy link
Contributor Author

Nerian commented Mar 7, 2014

That's an interesting thought. Maybe we need a super – or merge: true – option?

@dblock
Copy link
Member

dblock commented Mar 7, 2014

I think anything that says super will be clear(er).

@Nerian
Copy link
Contributor Author

Nerian commented Mar 12, 2014

Can we merge this? The old behaviour was merging with parent, so this fixes a regression bug. We can change the behaviour on a different PR.

@dblock
Copy link
Member

dblock commented Mar 12, 2014

I will merge this since it's a regression. @Nerian would you be so kind to open a new issue for changing this behavior?

dblock added a commit that referenced this pull request Mar 12, 2014
Test that you can reopen nested exposures
@dblock dblock merged commit d904381 into ruby-grape:master Mar 12, 2014
@Nerian Nerian deleted the cannot_reopen_exposure branch March 12, 2014 15:45
@Nerian
Copy link
Contributor Author

Nerian commented Mar 12, 2014

Sure thing. I created it here: #64

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

Successfully merging this pull request may close these issues.

None yet

3 participants