Representers #413

kzaitsev · 2016-05-07T14:19:48Z

This PR contains:

allow to attach custom model parser via GrapeSwagger.register_model_parser
trailblazer representers support
model parsers moved to separated classes
grape-entity not a runtime dependency

TODO:

changelog
update README

kzaitsev · 2016-05-07T15:14:53Z

Also related:

Support generating documenation from Roar presenters next to grape-entity #121

This allows to create gem with custom model parsers, like representable, roar, grape-entity, activemodel-serializer, virtus-model

https://github.com/Bugagazavr/grape-swagger-representable

@dblock

dblock · 2016-05-07T20:21:29Z

lib/grape-swagger/models/representable.rb

+end
+
+if defined?(::Representable)
+  GrapeSwagger.register_model_parser(GrapeSwagger::Models::Representable, ::Representable::Decorator)


I think this could be more elegant with something like GrapeSwagger.model_parsers.register ....

dblock · 2016-05-07T20:24:15Z

Overall, big 👍 on this. Looking forward to seeing this finished.

We could take grape-entity and representable parts out of grape-swagger altogether into separate gems instead of keeping support here or not.

kzaitsev · 2016-05-07T23:02:49Z

@dblock im add model parsers collection, but many specs uses grape entities in test.

I create two gems:

Should i publish this gems before this merged?

dblock · 2016-05-08T00:15:18Z

I think you should and make it very clear in UPGRADING in grape-swagger what users are supposed to do if they use one or the other. I would move specific tests into those projects and leave some basic tests that work for either library in this PR. You could also have a .travis.yml that sets something like ENTITY_LIBRARY and runs "integration" tests with it, a little bit how https://github.com/dblock/slack-ruby-client for example switches between faye-websocket and celluloid to test.

dblock · 2016-05-08T00:16:39Z

We don't have to, but if you would like to move grape-swagger-* into the ruby-grape organization we can do that too. I can make you co-maintainer of all three, grape-swagger and those two, lmk.

kzaitsev · 2016-05-08T01:59:22Z

@dblock ok, im agreed.

I squash my commits, add some information to readme and update changelog.

For specs uses grape-swagger-entity

.travis.yml also updated in https://github.com/Bugagazavr/grape-swagger-representable and https://github.com/Bugagazavr/grape-swagger-entity

So, now if this this is not problem, we can move this repos to ruby-grape namespace, and delete git ref from Gemfiles to my grape-swagger and add dependencies to gempecs

kzaitsev · 2016-05-08T02:05:36Z

ah, ok i forgot add ENTITY_LIBRARY, take a moment i add this

dblock · 2016-05-09T10:52:06Z

CHANGELOG.md

@@ -7,6 +7,7 @@
 * [#405](https://github.com/ruby-grape/grape-swagger/pull/405), [#403](https://github.com/ruby-grape/grape-swagger/issues/403): Added version support matrix - [@LeFnord](https://github.com/LeFnord).
 * [#408](https://github.com/ruby-grape/grape-swagger/pull/408): Added support for `HEAD` endpoints - [@Bugagazavr](https://github.com/Bugagazavr).
 * [#408](https://github.com/ruby-grape/grape-swagger/pull/411): Added support for `OPTIONS` endpoints - [@Bugagazavr](https://github.com/Bugagazavr).
+* [#413](https://github.com/ruby-grape/grape-swagger/pull/413): Move all model parsing logic to separate gems `grape-swagger-entity` and added representable parser `grape-swagger` - [@Bugagazavr](https://github.com/Bugagazavr)


Missing a period at the end :)

dblock · 2016-05-09T11:01:53Z

This is very close, see my smallish comments. I don't think we should treat grape-entity as "default" in specs basically. In the end travis would run general specs for many versions of Ruby and only once against each entity library on top of the latest ruby version or something like that.

dblock · 2016-05-09T11:02:39Z

I'm going to cut a release of grape-swagger with whatever changes we have on HEAD now FYI, that should give us more time to announce/release this big change.

dblock · 2016-05-09T11:04:13Z

README.md

@@ -46,7 +58,7 @@ grape-swagger | swagger spec | grape                   | grape-entity
 0.10.5        |     1.2      | >= 0.10.0 ... <= 0.14.0 |  < 0.5.0
 0.11.0        |     1.2      |               >= 0.16.2 |  < 0.5.0
 0.20.1        |     2.0      | >= 0.12.0 ... <= 0.14.0 | <= 0.5.1
-0.20.3 (next) |     2.0      | >= 0.12.0 ... <= 0.16.2 | <= 0.5.1
+0.21.0 (next) |     2.0      | >= 0.12.0 ... <= 0.16.2 | <= 0.5.1


Add a column here for representable.

kzaitsev · 2016-05-09T19:07:25Z

Ok, i create a simple mock shared example to launch tests without any entity library, and add 2 shared examples to tests grape-swagger with representable and grape-entity.

If all ok, i squash my commits into one.

dblock · 2016-05-10T12:57:07Z

README.md

+gem 'grape-swagger-representable'
+```
+
+##### Custom model parser interface


Make capitalization consistent, just "Custom Model Parsers"

dblock · 2016-05-10T13:00:47Z

I made a bunch of nitpick comments in README.

Could you please update https://github.com/ruby-grape/grape-swagger/blob/master/UPGRADING.md?

Squash, I'll merge. Thanks for the excellent work.

kzaitsev · 2016-05-10T19:00:21Z

@dblock done

dblock · 2016-05-10T19:55:07Z

Fix the build pls.

dblock · 2016-05-10T19:55:51Z

Gemfile

@@ -8,3 +8,5 @@ when 'HEAD'
 else
  gem 'grape', version
 end
+
+gem ENV['MODEL_PARSER'], github: "bugagazavr/#{ENV['MODEL_PARSER']}" if ENV.key?('MODEL_PARSER')


Lets move the "bugagazavr/" part into the ENV variable?

dblock · 2016-05-10T19:56:51Z

Will hit merge on 💚.

dblock · 2016-05-10T19:58:02Z

I see #415, this is cool, I'll just merge and deal with it.

dblock · 2016-05-10T20:02:09Z

Feel free to address any of my minor comments in a future PR. You should e-mail the grape mailing list about this change asking people to give it a shot.

dblock · 2016-05-10T20:02:31Z

Also release the representer libraries and change the MODEL_PARSER env to the gem name instead of a github reference.

kzaitsev · 2016-05-11T03:59:43Z

Thanks 👍

https://rubygems.org/gems/grape-swagger-entity
https://rubygems.org/gems/grape-swagger-representable

What about move this gems to ruby-grape org?

dblock · 2016-05-11T16:41:18Z

@Bugagazavr lets do it offline

dblock · 2016-05-11T16:41:31Z

@Bugagazavr email me dblock at dblock dot org

Representers

kzaitsev force-pushed the representers branch from 0de8fb5 to d27515e Compare May 7, 2016 14:23

dblock reviewed May 7, 2016
View reviewed changes

kzaitsev force-pushed the representers branch 3 times, most recently from ce94702 to d7d6b1b Compare May 8, 2016 01:48

kzaitsev force-pushed the representers branch 4 times, most recently from 4c19f5e to da93d16 Compare May 8, 2016 12:12

dblock reviewed May 9, 2016
View reviewed changes

kzaitsev force-pushed the representers branch 4 times, most recently from 1ecfd6b to e6934df Compare May 9, 2016 16:17

kzaitsev force-pushed the representers branch 2 times, most recently from 78f395f to 9942e12 Compare May 9, 2016 16:19

dblock reviewed May 10, 2016
View reviewed changes

Move model parsing logic outside grape-swagger

f4a0c15

kzaitsev force-pushed the representers branch from 7396b1b to f4a0c15 Compare May 10, 2016 18:59

dblock reviewed May 10, 2016
View reviewed changes

dblock merged commit 70a31d3 into ruby-grape:master May 10, 2016

LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019

Merge pull request ruby-grape#413 from Bugagazavr/representers

b27eed7

Representers

Representers #413

Representers #413

Uh oh!

Conversation

kzaitsev commented May 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzaitsev commented May 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblock May 7, 2016

Choose a reason for hiding this comment

Uh oh!

dblock commented May 7, 2016

Uh oh!

kzaitsev commented May 7, 2016

Uh oh!

dblock commented May 8, 2016

Uh oh!

dblock commented May 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzaitsev commented May 8, 2016

Uh oh!

kzaitsev commented May 8, 2016

Uh oh!

dblock May 9, 2016

Choose a reason for hiding this comment

Uh oh!

dblock commented May 9, 2016

Uh oh!

dblock commented May 9, 2016

Uh oh!

dblock May 9, 2016

Choose a reason for hiding this comment

Uh oh!

kzaitsev commented May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblock May 10, 2016

Choose a reason for hiding this comment

Uh oh!

dblock commented May 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzaitsev commented May 10, 2016

Uh oh!

dblock commented May 10, 2016

Uh oh!

dblock May 10, 2016

Choose a reason for hiding this comment

Uh oh!

dblock commented May 10, 2016

Uh oh!

dblock commented May 10, 2016

Uh oh!

dblock commented May 10, 2016

Uh oh!

dblock commented May 10, 2016

Uh oh!

kzaitsev commented May 11, 2016

Uh oh!

dblock commented May 11, 2016

Uh oh!

dblock commented May 11, 2016

Uh oh!

Uh oh!

kzaitsev commented May 7, 2016 •

edited

Loading

kzaitsev commented May 7, 2016 •

edited

Loading

dblock commented May 8, 2016 •

edited

Loading

kzaitsev commented May 9, 2016 •

edited

Loading

dblock commented May 10, 2016 •

edited

Loading