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

Let the OutputBuilder be the Hash as well #232

Merged
merged 1 commit into from
Jul 4, 2016

Conversation

sagebomb
Copy link
Contributor

#213
Hi everyone :suspect:
The problem that we've changed the Hash output to the OutputBuilder one (see #204) . Even though an OutputBuilder instance delegates everything to its output object (which is a Hash) - it returns false when we do this: .is_a?(Hash) on a OutputBuilder instance. is_a? method is not delegated. And grape errors formatters expect to get a Hash instance from Entity after representation, not OutputBuilder one.

I checked grape master branch and my grape-entity branch tests. Everything should be fine.

@sagebomb sagebomb force-pushed the fix-grape-master-falling-specs branch from 7d7797b to d96ef92 Compare June 30, 2016 21:51
@dblock
Copy link
Member

dblock commented Jul 1, 2016

The build is toast, but I don't think it's your fault. If you don't mind taking a look though please.

@sagebomb sagebomb force-pushed the fix-grape-master-falling-specs branch from d96ef92 to 1b791b2 Compare July 1, 2016 08:13
@sagebomb
Copy link
Contributor Author

sagebomb commented Jul 1, 2016

MRI 2.3: ✅
Lower ruby versions: Gem::InstallError: activesupport requires Ruby version >= 2.2.2.. I think it's happened because of activesupport (5.0.0) which was released yesterday. We can add <= 4.2.6 to gemspec for activesupport.

@dblock
Copy link
Member

dblock commented Jul 2, 2016

Yes please

@sagebomb sagebomb force-pushed the fix-grape-master-falling-specs branch 2 times, most recently from aa915f7 to e1624d9 Compare July 2, 2016 20:30
@sagebomb
Copy link
Contributor Author

sagebomb commented Jul 2, 2016

The same problem with rack and json. Looks good now.

@@ -4,6 +4,7 @@
* [#215](https://github.com/ruby-grape/grape-entity/pull/217): Fix: `#delegate_attribute` no longer delegates to methods included with `Kernel` - [@maltoe](https://github.com/maltoe).
* [#219](https://github.com/ruby-grape/grape-entity/pull/219): Fix: double pass options in serializable_hash - [@sbatykov](https://github.com/sbatykov).
* [#226](https://github.com/ruby-grape/grape-entity/pull/226): Add fetch method to fetch from opts_hash - [@alanjcfs](https://github.com/alanjcfs).
* [#232](https://github.com/ruby-grape/grape-entity/pull/232): Fix: grape tests are green [#213](https://github.com/ruby-grape/grape-entity/issues/213) - [@avyy](https://github.com/avyy).
Copy link
Member

Choose a reason for hiding this comment

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

This isn't material for users, so remove it.

Copy link
Member

Choose a reason for hiding this comment

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

However, the kind_of change is relevant, so you should say something to that regard.

@sagebomb
Copy link
Contributor Author

sagebomb commented Jul 4, 2016

@dblock check last changes pls

…class, specify rack, json and active_support version for supporting old rubies
@sagebomb sagebomb force-pushed the fix-grape-master-falling-specs branch from 8e11a2c to a6c8e85 Compare July 4, 2016 15:41
@dblock
Copy link
Member

dblock commented Jul 4, 2016

Merging, thanks.

@dblock dblock merged commit c5abc1b into ruby-grape:master Jul 4, 2016
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.

2 participants