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

Some subtle improvements #134

Merged
merged 3 commits into from
Jun 4, 2015
Merged

Some subtle improvements #134

merged 3 commits into from
Jun 4, 2015

Conversation

etehtsea
Copy link
Contributor

@etehtsea etehtsea commented Jun 4, 2015

Because less code is better than more.

expect(child_class.exposures).to eq(name: {})
end

it 'remove from parent and do not remove from child classes that have locked down their attributes with an .exposures call' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why the other test is no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is no more "confusing" behaviour. unexpose no longer affects child classes regardless of was .exposures method called in them or not.

It's kinda breaking change, but I hope nobody relied on such behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused, does this spec break now? I mean when you unexpose :email from the parent class, the child no longer has it?

In which case this at least needs an UPGRADING entry, although I think you want to track down the PR in which this was added and figure out why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from now, if you unexpose from parent class, child doesn't affect.

master:

[3] pry(main)> parent = Class.new(Grape::Entity)
=> #<Class:0x0000010250cf00>
[4] pry(main)> parent.expose :name, :email
=> [:name, :email]
[5] pry(main)> child = Class.new(parent)
=> #<Class:0x00000102a25938>
[6] pry(main)> child.exposures
=> {:name=>{}, :email=>{}}
[7] pry(main)> child_leaking = Class.new(parent)
=> #<Class:0x0000010292fab0>
[8] pry(main)> # not called .exposures
[9] pry(main)> parent.unexpose :email
=> {}
[10] pry(main)> child.exposures
=> {:name=>{}, :email=>{}}
[11] pry(main)> child_leaking.exposures
=> {:name=>{}}

my-branch:

[3] pry(main)> parent = Class.new(Grape::Entity)
=> #<Class:0x000001031fb108>
[4] pry(main)> parent.expose :name, :email
=> [:name, :email]
[5] pry(main)> child = Class.new(parent)
=> #<Class:0x000001032a0888>
[6] pry(main)> child.exposures
=> {:name=>{}, :email=>{}}
[7] pry(main)> child_leaking = Class.new(parent)
=> #<Class:0x0000010319f588>
[8] pry(main)> # not called .exposures
[9] pry(main)> parent.unexpose :email
=> {}
[10] pry(main)> child.exposures
=> {:name=>{}, :email=>{}}
[11] pry(main)> child_leaking.exposures
=> {:name=>{}, :email=>{}}

It was introduces in f089f16 and I think comment

# the following 2 behaviors are testing because it is not most intuitive and could be confusing

explains why. Because exposures merge happened in children classes only when it was called.

I'll add entry to UPGRADING.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find UPGRADING, so added note to CHANGELOG

Get rid of confusing behaviour when `unexpose` could leak to subclasses
which have not locked down their attributes with `.exposures` call yet.
@dblock
Copy link
Member

dblock commented Jun 4, 2015

Ok, I am good with this, merging. Thank you very much.

dblock added a commit that referenced this pull request Jun 4, 2015
@dblock dblock merged commit 11914e8 into ruby-grape:master Jun 4, 2015
@etehtsea etehtsea deleted the refactoring branch June 4, 2015 18:13
@dblock
Copy link
Member

dblock commented Jun 4, 2015

Check out https://github.com/intridea/grape-entity/pulls too, there're a few bugs that were raised that sound vaguely familiar, maybe this fixes anything.

@marshall-lee
Copy link
Member

I think more and more that #161 and #166 are caused by this patch.

#166 is easy to fix
but for #161: superclass traversal was an intended thing but now it's removed. Issue with unexpose on parent class should be fixed different way.

marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.

Conflicts:
	.rubocop_todo.yml
	CHANGELOG.md
	lib/grape_entity/entity.rb
	lib/grape_entity/version.rb
	spec/grape_entity/entity_spec.rb
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.

Conflicts:
	.rubocop_todo.yml
	CHANGELOG.md
	lib/grape_entity/entity.rb
	lib/grape_entity/version.rb
	spec/grape_entity/entity_spec.rb
marshall-lee added a commit to marshall-lee/grape-entity that referenced this pull request Aug 10, 2015
This fixes a regression introduced after merging a ruby-grape#134.
It's something undocumented before but it worked before and I don't see
anything harmful in the presence of this feature.

Fixes ruby-grape#166.
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.

3 participants