Skip to content

implement universally unique attributes to silo exposure logic #90

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
Closed
wants to merge 5 commits into from
Closed

implement universally unique attributes to silo exposure logic #90

wants to merge 5 commits into from

Conversation

wyattisimo
Copy link
Contributor

Here's my first stab at issue #52. It silos exposure logic by creating a universally unique attribute for each exposure.

It allows for stuff like this to work properly:

expose :name, if: { greeting: :formal } do |obj, opts|
  "#{obj.title} #{obj.first_name} #{obj.last_name}"
end

expose :name, if: { greeting: :friendly } do |obj, opts|
  "#{obj.nick_name}"
end

This ended up requiring more tweaking than I expected. I also had to modify a number of specs that are accessing certain protected methods that now require a unique attribute (like value_for).

I added specs to test applying "if" conditions to an attribute in separate exposure declarations, and all existing specs are currently passing, but there are likely some new use cases and/or side effects that need to be discussed/resolved.

EDIT

Just for clarification... using the above exposure declarations, grape-entity currently produces the following exposures hash:

{
  name: {
    if: { greeting: :friendly },
    proc: #<Proc2>
  }
}

With this update, those same exposure declarations will result in an exposures hash like this:

{
  name___939828ec686b4a6cb9f86ddbe38439a3: {
    if: { greeting: :formal },
    proc: #<Proc1>
  },
  name___b93f9ab39e4c4b67a6aa192cd7f49dbf: {
    if: { greeting: :friendly },
    proc: #<Proc2>
  }
}

dblock
dblock reviewed Sep 7, 2014
View reviewed changes
spec/spec_helper.rb
@@ -12,3 +12,25 @@
RSpec.configure do |config|
config.raise_errors_for_deprecations!
end

module Grape
Copy link
Member

Choose a reason for hiding this comment

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

For sanity sake we should organize these things into spec/support, so spec/support/grape/entity.rb.

@dblock
Copy link
Member

dblock commented Sep 7, 2014

I think this looks really good. It needs possibly README and definitely CHANGELOG updates, and a squash, please.

@dblock
Copy link
Member

dblock commented Sep 7, 2014

I wonder whether an implementation that stores an array of exposures instead of uniquely labeled ones would be cleaner. This way you would have a Hash of attribute => exposures. Either way this is a good step forward, especially that there're now tests for this kind of stuff.

@dblock
Copy link
Member

dblock commented Nov 18, 2014

Bump @wyattisimo.

@wyattisimo
Copy link
Contributor Author

@dblock Sorry, been a bit busy. I think you're right about an array of exposures being cleaner. I'll get to it this week.

@swistaczek
Copy link

@wyattisimo hey, looks great 👍 . Looking forward for updates 😄.

@wyattisimo
Copy link
Contributor Author

@dblock finally had time to get back to this. implemented array of exposures for each attribute. all tests are passing.

@wyattisimo
Copy link
Contributor Author

@dblock okay, upstream conflicts should be resolved. let me know what you think.

i'll squash and update README and CHANGELOG once it's finalized

@dblock
Copy link
Member

dblock commented Mar 16, 2015

This looks good to me, squash/rebase/etc.?

@u2
Copy link
Contributor

u2 commented Mar 26, 2015

What's going on?

@dblock
Copy link
Member

dblock commented Mar 26, 2015

There're quite a few merge conflicts that need to be looked at carefully, bump @wyattisimo.

@wyattisimo
Copy link
Contributor Author

Sorry for the delay! Been busy with other things and stuff and things... I'll try to get this wrapped up in the next few days.

@u2
Copy link
Contributor

u2 commented Apr 17, 2015

Maybe I can help to squash these commits, there are quite a few merge conflicts and need to change something for the new commits.

@u2 u2 mentioned this pull request May 10, 2015
Merged
@marshall-lee
Copy link
Member

@wyattisimo, @u2

Hi! Please look at the work done in #151 — it solves this problem differently and without performance degradation. It's not released yet and will go to 0.5.0 but you can try it out from current master.

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.
5 participants
@wyattisimo @dblock @swistaczek @u2 @marshall-lee