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

on code design #2

Closed
JorisBenschop opened this issue Jun 27, 2017 · 12 comments
Closed

on code design #2

JorisBenschop opened this issue Jun 27, 2017 · 12 comments

Comments

@JorisBenschop
Copy link
Contributor

Hi,
I really like that you started this project. I also understand this is a showcase to get the DRF people to incorporate proper swagger annotation in their core code. I dont mind to contribute to this code, but I'm not sure if it is your intention to extend this code at all or just use it as a demo. If not, there may not be a point in working on this project.

On a more specific not, I noticed that you extend the coreschema module during import of your coreschema_schemas.py. This is certainly an unusual (and maybe unstable) trick, but it also breaks the grouping in swagger endpoints. Also I'm not sure why you decided to put the coreschema object definitions inside your schemaview, as it seems this information is specific for each endpoint and is better located at the endpoint definition.

Again, I'm not trying to tell you what to do, I truely appreciate your effort and I hope I can contribute here.

best
joris

@tovmeod
Copy link
Owner

tovmeod commented Jun 27, 2017

Hi,

Thanks for the encouragement!

This is not only a showcase, I'm using it in 2 projects already.

Help is greatly appreciated, I understand that this project may live a long time, even a trivial thing I opened like (encode/django-rest-framework#5224) is taking a long time.

I noticed that you extend the coreschema module ...

Can you be more specific on what are you talking about? I understand that the monkey patching is an unstable trick, but how this breaks the grouping?

The idea of putting the object definitions inside the schemaview is only user defined extra definitions, one need only to inherit one class and may configure and extend everything needed in one place.

@JorisBenschop
Copy link
Contributor Author

I dont know how the grouping breaks to be honest, I just see that my endpoints are no longer in their 'subfolder' structure but all in one big list (oddly the subfodlers are also still there). Now I know that you are serious about maturing this project I will spend more time on it (after my holidays :)
I'm still a bit struggling with the Meta class and which code actually reads out this code. Hopefully I will have time to analyze the code a bit more soon.

Could we not subclass coreschema instead of patching it (BetterCoreSchema)? Or will this then not be picked up by DRF?

I'm also not really sure how you use subclassing for the schemavieew. The way I understand is that this instance is only used once in the /docs/ URL. Again, I dont have a full understanding of DRF and coreapi with the exception that I got it to work and it sortof works and doesnt crash.

@tovmeod
Copy link
Owner

tovmeod commented Jun 27, 2017

by grouping do you mean tags?

I did change one thing about that, maybe this is what affects you: https://github.com/tovmeod/drf-swagger-missing/blob/master/drf_swagger_missing/openapi_codec_encode.py#L14

if this is problematic to you I believe this behavior could be conditional, we may add a flag to still add only one tag.

You can search for the string 'meta' and you will find where it reads from it.
In any case I only use it in the schema generator. see https://github.com/tovmeod/drf-swagger-missing/blob/master/drf_swagger_missing/rest_framework_schemas.py

I can't subclass coreschema since it is a module and not a class.

About the schemaview, yes, it is used only in your project urls.py once, you may give whatever path you want just like any other view in the project, you need to subclass if you want to configure title, description, version and/or add extra schema definitions or overwrite one of the generated ones

I'll be happy to explain anything to you

@JorisBenschop
Copy link
Contributor Author

I'll try to make screenshots. I'm not sure if tags is what I mean.

Do you have any idea on a way to apply this code to function based views? This is 99% of what we use here internally

@JorisBenschop
Copy link
Contributor Author

also, my code crashes with GET requests on this line. It seems I cannot seem to set schema.default. Probably i dont understand it enough.

@tovmeod
Copy link
Owner

tovmeod commented Jun 27, 2017

MySwaggerView.as_view() returns a function, you may use it alongside with other function based views:

url(r'^docs/api/$', MySwaggerView.as_view()),

is that a problem?

Previously I was editing the swagger.yml manually, so some naming convention I use comes from there
I'm using http://editor.swagger.io/#/ to debug the actual generated definition, you may get yours by appending '?format=openapi', so if you used a path like the suggested above you can go to: http://127.0.0.1:8000/docs/api/?format=openapi and get a JSON definition. Paste that on the swagger editor and it will ask to convert to yml.

can you send me the traceback of your error?

@JorisBenschop
Copy link
Contributor Author

what I meant was: previously we annotated FBV using yml inside the docstring. With coreapi this is no longer possible.

@tovmeod
Copy link
Owner

tovmeod commented Jun 27, 2017

I understand the problem.
If you were starting a project I would recommend using viewsets, you would gain much such as object definitions and common responses like a list of the serializer type for list()

but you have an existing project, it is not feasible to refactor all of it just to be able to document.

I'm thinking that instead or on top of the Meta class one could use a decorator to define the responses and extra fields to the view

@Responses([Response(), Response()])
def func_view(...)
pass

so the generator can get using: view.responses instead of view.Meta.responses[method_name]

would that work for you?

Can you test if it is possible to add attributes to a function using a decorator? would you mind writing the decorator? we would still need to make sure this approach works with CBV

@JorisBenschop
Copy link
Contributor Author

I think your decorator idea is very elegant.
In my projects we deliberately dont use CBVs because we often combine data from many different models into a single view. CBVs then become difficult to maintain. But I agree: lets first focus on CBV to get this into production.

@tovmeod
Copy link
Owner

tovmeod commented Jun 27, 2017

thanks, but elegant doesn't cont if it doesn't work :)

90% of the work I did was to be able to generate the same swagger I had created by hand, I think that if we are able to to the same with your project it would greatly help the both of us and enable the project to be useful to others, you are not the only one using fbv's

@tovmeod
Copy link
Owner

tovmeod commented Jun 29, 2017

So it seems the DRF schema generator will ignore fbv's, among others see rest_framework.schemas.is_api_view:

def is_api_view(callback):
"""
Return True if the given view callback is a REST framework view/viewset.
"""
cls = getattr(callback, 'cls', None)
return (cls is not None) and issubclass(cls, APIView)

were you able to solve this?

@JorisBenschop
Copy link
Contributor Author

I'm still trying to get a better grasp of the inner workings of DRF. Also my holiday just started, so my input is limited for the next few weeks. I'll try to squeeze some time in where I can

@tovmeod tovmeod closed this as completed Oct 26, 2017
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

No branches or pull requests

2 participants