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

default value for path field should be empty string instead of None #5224

Closed
wants to merge 1 commit into from

Conversation

tovmeod
Copy link

@tovmeod tovmeod commented Jun 19, 2017

Description

the schema_cls doesn't pass the default value for the field, so it will be None, meaning swagger ui will show 'null' when it should be empty

@xordoquy
Copy link
Collaborator

It seems it's not as simple as that since it breaks a few tests.

@tovmeod
Copy link
Author

tovmeod commented Jun 21, 2017

if this is something that can be accepted I can invest the time to adjust the tests that broke

@xordoquy
Copy link
Collaborator

Good point.
Maybe a bit more context about the initial issue ? a screenshot maybe ?

@tovmeod
Copy link
Author

tovmeod commented Jun 22, 2017

default value for default argument is None.
None is rendered as null, so swagger ui will be like this:

image

setting default='' it will look like this:

image

@tomchristie
Copy link
Member

Okay, so I think we need a slightly different tack to this, even if might seem obvious on first sight.

Our current internal representation for a path field is "has no default, is required" which is exactly what we want. What's awkward is how swagger UI chooses to display that. My opinion is that we should instead change how we map this case onto a Swagger representation, rather than break our underlying Core API representation.

@tovmeod
Copy link
Author

tovmeod commented Jul 11, 2017

Well, None is rendered as null in JSON, so I believe the renderer is correct, the UI renders null as 'null'. this could be an issue with swagger UI, but right now I have my hands on the python side of things.

We could change the generator to not include the default attribute if the value is None, this would cause the UI to not display a default value, the same as it shows an empty string, but what would happen if someone actually wants null to be the default value?

@Igonato
Copy link

Igonato commented Aug 17, 2017

Looks like a django-rest-swagger issue. It should take this into account during CoreAPI -> OpenAPI conversion

@carltongibson
Copy link
Collaborator

I'm going to close this. There may be issue(s) with (a) particular renderer(s) but that's not here with the autogeneration of path fields. Happy to review better targeted issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants