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

Support present hash #956

Merged
merged 1 commit into from
Mar 19, 2015
Merged

Support present hash #956

merged 1 commit into from
Mar 19, 2015

Conversation

u2
Copy link
Contributor

@u2 u2 commented Mar 13, 2015

if key
representation = (@body || {}).merge(key => representation)
elsif entity_class.present? && representation.respond_to?('merge')
representation = (@body || {}).merge(representation)
elsif hash_present && representation.respond_to?('merge')
Copy link
Member

Choose a reason for hiding this comment

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

Should this be respond_to?(:merge)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, removed already.

@dblock
Copy link
Member

dblock commented Mar 13, 2015

So, the real question given this code is why isn't present <hash> a noop to return the hash? What are we gaining by running it through present?

@dblock
Copy link
Member

dblock commented Mar 13, 2015

Another question is what does present <hash>, with: Presenter do and does it make any sense?

@u2
Copy link
Contributor Author

u2 commented Mar 14, 2015

Yes, present <hash> just merge body and return the body, no other thing.

@u2
Copy link
Contributor Author

u2 commented Mar 14, 2015

Yes, it's better to present <hash>, with: Presenter.I will try to rewrite it.

@dblock
Copy link
Member

dblock commented Mar 15, 2015

I think I am missing something. What is the difference in the result between present <hash> and present <hash>, with: Presenter?

@u2
Copy link
Contributor Author

u2 commented Mar 15, 2015

For the result, there is no difference.But present <hash>, with: Presenter fit the grape style.Just add a Presenter, and use with: Presenter for represent the object itself.Also, it can present anything, not only Hash.

@dblock
Copy link
Member

dblock commented Mar 15, 2015

Hm, I can see the argument for a noop class like this. Do you think it belongs in Middleware? It's not a Middleware.

(and sorry to be difficult :)

@u2
Copy link
Contributor Author

u2 commented Mar 15, 2015

I was hesitant to about that, and I am not sure where I can place Presenter.

@dblock
Copy link
Member

dblock commented Mar 15, 2015

Maybe just Grape::Presenters::Presenter?

@u2
Copy link
Contributor Author

u2 commented Mar 15, 2015

Ok, it's a good idea.I will change it right now. :).

end

it 'presents both dummy presenter' do
puts subject.body.inspect
Copy link
Member

Choose a reason for hiding this comment

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

Stray puts here.

Lets put this into a separate _spec file too.

@dblock
Copy link
Member

dblock commented Mar 18, 2015

I think the code here looks perfect. This needs an entry in README. Maybe just describe something around using this presenter to keep things consistent?

@u2
Copy link
Contributor Author

u2 commented Mar 18, 2015

I have updated README.

@dblock
Copy link
Member

dblock commented Mar 19, 2015

Merging, thanks for your patience.

dblock added a commit that referenced this pull request Mar 19, 2015
@dblock dblock merged commit 44914da into ruby-grape:master Mar 19, 2015
@swistaczek
Copy link

Hey, I still dont get idea behind this change 💃

@dblock
Copy link
Member

dblock commented Apr 2, 2015

It's entirely to make returning data consistent, if you present an object you should also be able to present anything else. Sometimes it makes sense if your code path figures out what to present, then presents it.

@u2 u2 deleted the present_hash branch April 5, 2015 03:10
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