-
Notifications
You must be signed in to change notification settings - Fork 264
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
DjangoFilters: Add description for enum choices #403
Conversation
Codecov Report
@@ Coverage Diff @@
## master #403 +/- ##
==========================================
+ Coverage 98.51% 98.55% +0.03%
==========================================
Files 55 55
Lines 5394 5388 -6
==========================================
- Hits 5314 5310 -4
+ Misses 80 78 -2
Continue to review full report at Codecov.
|
hi @valentijnscholten, that is a very nice idea and it has come up before in #337 as a matter of fact i do like this from a UI perspective, but i would prefer to have a structured approach via OpenAPI. others have raised this pain point in OAI/OpenAPI-Specification#681. maybe we should show some support there. unfortunately it looks like it didn't make into 3.1. |
OK, didn't see that. I got this info from the official swagger docs, so I was expecting it to be common. Since it's just a few lines of code, I feel it wouldn't hurt to have this merged until (maybe in the distant future) there's a more structured solution from OA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is true that this would would most likely not break anybody.
we are still waiting for adoption of 3.1 in upstream projects like swagger-ui. i suppose 3.1.1 or 3.2 support is not gonna be available in the near future. with that said it's probably reasonable to support this in the meantime. it's just tricky to take features out again when the time comes, but the benefits probably outweigh the risk.
if we gonna move forward here, we would also need this for the non django-filter case. maybe factor out the the string generation into plumbing.py
so we can use it in the other places too.
@@ -121,6 +121,12 @@ def resolve_filter_field(self, auto_schema, model, filterset_class, field_name, | |||
elif filter_field.label is not None: | |||
description = filter_field.label | |||
|
|||
if 'choices' in filter_field.extra: | |||
description = description or '' | |||
description += '>\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the
>
looks weird. - we should add an empty line as separation if there is an description
- i don't like the way the empty backticks look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the '>' and backticks where part of the official format needed, but it looks like they can be removed/changed, committed that. I did add the '\n' but it doesn't seem to do anything for the json spec, not even sure if json supports multiline strings".
My first attempt was by modifying the build_choice_field
in plumbing.py
, but these fields from DjangoFilters don't seem to pass through there. Or do you suggest to add code there as well as keeping it in the django filter part?
I have extracted the description generator code and added a call in
I am happy to help out further, but don't really have the time to fully dive into all the schema generation code in here. Do you maybe have some pointers of where this description would be best construct to fix these NOK cases @tfranzel ? |
hi @valentijnscholten, looks nice! please squash together the commits and force push for a clean history. i'll take over from there. can't really give pointers here. i would have to investigate myself on how to resolve the remaining issues. i'm pretty strapped for time at the moment. it will take a few days for me to get to it. |
543bb8a
to
0c42e21
Compare
0c42e21
to
69d3a14
Compare
sqaushed. thanks for looking into it, no rush, good to know it will eventually be implemented. |
Thanks to @valentijnscholten for the first attempt in #403
superseded in favor of #952. @valentijnscholten thank you for this PR. There were a couple more things to address with this feature, namely the postprocessing hook and non-django-filter behavior. Took me a while to get around to it. Please review this other PR whtether it adresses your points. Would be great if you could do it by end of the week, so that we can do a pending release. |
It's been almost two years since I did anything in API spec related stuff in django, so I'm going to rely on you for this one :-) |
Ahh ok, sry you won't benefit from this anymore. Have a good one. |
Defect Dojo is still using it, so there will be benefit :-) Thanks for implementing the suggested enhancements. |
Thanks to @valentijnscholten for the first attempt in #403
The choices in a ChoiceFilter (enum) can be documented via the description, similar to what's already in place for OrderingFilter.
This will help users, for example in SwaggerUI.
No idea if this is the best way / place to implement this, but it works locally at least.
Before:
After: