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

Fix incorrect data type linking for request params of entity types #511

Merged
merged 1 commit into from
Oct 13, 2016

Conversation

serggl
Copy link
Member

@serggl serggl commented Oct 3, 2016

This is a fix for #507.

Few moments were changed:

  1. Refactored to use a single method for parsing entity names (see recent comments to corrects exposing of inline definitions #503)
  2. Custom parameter types are now parsed when parameter objects are parsed. This is new. Previously all custom types were parsed at a later steps. Not sure if we need a models option in add_swagger_documentation method from now on, since it looks mostly like a workaround
  3. changed links to custom types from "type": "TypeA" to "$ref": "#/definitions/TypeA"

serggl added a commit to anjlab/grape-swagger that referenced this pull request Oct 3, 2016
@serggl serggl force-pushed the bugfix/param-types-definition branch from 14181e7 to 885a9ef Compare October 3, 2016 18:38
serggl added a commit to anjlab/grape-swagger that referenced this pull request Oct 3, 2016
@JanStevens
Copy link

One question, why is the length set to 42? Is this somewhere in the json schema spec (or swagger spec)?

@serggl
Copy link
Member Author

serggl commented Oct 10, 2016

great question, @JanStevens ! the answer is - no Idea :) this is a part of current code (https://github.com/ruby-grape/grape-swagger/pull/511/files#diff-7af39528837333c960830618fb6a52e9L297) which is not documented

@JanStevens
Copy link

JanStevens commented Oct 10, 2016

Yes I know,

The reason why I'm asking is that our current project has different namespaces and internal and public apis. So our entities end up being AnimalFarm::Internal::CowEntity current code strips this all down to AnimalFarmInternal (not really useful).

My current fix is to define a entity_name class method that just returns the name of the class, that works ofcourse but isn't really handy.

@serggl
Copy link
Member Author

serggl commented Oct 10, 2016

Can someone from maintainers bring some light here?

@dblock
Copy link
Member

dblock commented Oct 10, 2016

We have to ask @LeFnord, this was added in 054c54f it looks like.

@LeFnord
Copy link
Member

LeFnord commented Oct 10, 2016

Yeap, I did it, cause 42 is the answer of all 😉 …
but seriously, the intention of limiting it was only to have a readable definition name,
which could be very long in some cases, but it seems not long enough …

will create a PR tonight, which remove the limit, ok?

@JanStevens
Copy link

Maybe also document the entity_name option? By default I expect it to be class.name and if I want it differently I start adding entity_names to my entities

@LeFnord
Copy link
Member

LeFnord commented Oct 11, 2016

one night later … #515 removes the limit

@serggl … can you rebase your PR …

@serggl
Copy link
Member Author

serggl commented Oct 12, 2016

@LeFnord sure I can. Right after you guys merge #515?

@serggl serggl force-pushed the bugfix/param-types-definition branch from b160428 to 75a79ae Compare October 13, 2016 13:21
@serggl serggl force-pushed the bugfix/param-types-definition branch from 75a79ae to 5c76186 Compare October 13, 2016 13:59
@LeFnord
Copy link
Member

LeFnord commented Oct 13, 2016

thanks @serggl … good job 😄

@LeFnord LeFnord merged commit d7b7465 into ruby-grape:master Oct 13, 2016
LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
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.

4 participants