Avoid empty hashes #420
Avoid empty hashes #420
Conversation
| properties = parser.call unless parser.nil? | ||
|
|
||
| if parser.nil? | ||
| properties = { unsupported_model_parser: { type: :string, description: "No parser registed for #{model_name}" } } |
There was a problem hiding this comment.
If this is capitalized, it should be a full sentence, so ends with a period, please.
|
Seems a little weird to get an error inside the model's JSON output. Shouldn't it just fail and error out if developer action is required? |
ok, im just add |
kzaitsev
force-pushed
the
avoid-empty-hashes
branch
3 times, most recently
from
24d2094
to
67f4f7c
Compare
|
@dblock done, added two exceptions Also added mock parser to passing tests without external parser library. Thanks again, and excuse me for the lack of points in the end of sentences. |
| @@ -8,7 +8,8 @@ | |||
| #### Fixes | |||
|
|
|||
| * [#416](https://github.com/ruby-grape/grape-swagger/pull/416): Support recursive models - [@lest](https://github.com/lest). | |||
| * [#418](https://github.com/ruby-grape/grape-swagger/pull/418): Replaced github ref to rubygems for external gems - [@Bugagazavr](https://github.com/Bugagazavr). | |||
| * [#419](https://github.com/ruby-grape/grape-swagger/pull/419): Replaced github ref to rubygems for external gems - [@Bugagazavr](https://github.com/Bugagazavr). | |||
| * [#420](https://github.com/ruby-grape/grape-swagger/pull/420): Raise SwaggerSpec exception if swagger spec doesn't satified, when no parser for model registred or response model is empty - [@Bugagazavr](https://github.com/Bugagazavr). | |||
There was a problem hiding this comment.
Spelling: satiSfied, say "isn't satisfied".
|
Very good. See comments. English is also my third language so no stress, you're doing great ;) |
kzaitsev
force-pushed
the
avoid-empty-hashes
branch
from
67f4f7c
to
4cac1a9
Compare
|
@dblock i'l merge this, ok? |
|
Merged, looks good. |
|
The extraction of the parser and introducing the GrapeSwagger::Errors::UnregisteredParser breaks usage for every user. Don't you think this should have been at least a minor version increase not only a patch from 0.20.3 to 0.20.4? To be honest, currently everything seems to break for me with every minor version. You are moving really really fast here. |
|
Inside the https://github.com/ruby-grape/grape-swagger/blob/master/UPGRADING.md it is also refered to as a change for 0.21.0 |
|
sorry, i forgot change upgrading guide. |
|
By the way to get me right, I don't think this is a good change for 0.20.3 -> 0.20.4. But it should instead be 0.21.0 as it breaks usage for customers! |
|
Yes this is breaking changes. @dblock should we yank this release and publish this changes as 0.21.0? |
|
If you think it breaks everyone, yank it. |
|
@Bugagazavr yeap, think it would be good to yank the release, not only of breaking changes, also the support for grape > 0.16.x was announced for 0.21.0 release, but before I want to fix most bugs, to have a (relatively) stable version, which works for most cases 😉 think, we should have a better coordination 😄 |
|
Agreed, sorry for this, I yank 0.20.4 soon as possible Sent from my iPhone
|
|
no problem |
|
@LeFnord @Bugagazavr Feel free to improve the organizational aspects of this project in whichever way you see fit. However remember that the only thing we ask from contributors is their best effort, aka we ask nothing. Roadmaps tend to become commitments - I suggest replacing them with communicating intent via issues, READMEs and such instead. |
|
@dblock this is meant with … the main goal should be that we go into the same direction 😉 |
Avoid empty hashes
Problem:
currently returned empty hash if model parser doesn't registered, swagger-ui doesn't check json object, for empty, and get a null, swagger-ui with swagger 2.0 spec doesn't support null's and not loaded.
Solution:
raise exception if parser for current model not registered or model parsing result contains empty hash, to specify this problem for developer