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

More docstrings #107

Closed
jayvdb opened this issue Jun 30, 2020 · 6 comments
Closed

More docstrings #107

jayvdb opened this issue Jun 30, 2020 · 6 comments

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Jun 30, 2020

Followup from #96 , now I am getting the following appear in my schema

six (6) times:

description: Concrete view for retrieving, updating a model instance.

24 times:

description: Concrete view for retrieving, updating or deleting a model instance.

And others

> grep 'Concrete view' public/schema.yaml | wc -l
73

Guess where they comes from... ;-)

django-rest-framework> git grep 'Concrete view'
rest_framework/generics.py:# Concrete view classes that provide method handlers
rest_framework/generics.py:    Concrete view for creating a model instance.
rest_framework/generics.py:    Concrete view for listing a queryset.
rest_framework/generics.py:    Concrete view for retrieving a model instance.
rest_framework/generics.py:    Concrete view for deleting a model instance.
rest_framework/generics.py:    Concrete view for updating a model instance.
rest_framework/generics.py:    Concrete view for listing a queryset or creating a model instance.
rest_framework/generics.py:    Concrete view for retrieving, updating a model instance.
rest_framework/generics.py:    Concrete view for retrieving or deleting a model instance.
rest_framework/generics.py:    Concrete view for retrieving, updating or deleting a model instance.

Maybe it is just some missing from the list of view bases in 57ecb4a

I suggest that instead of the very brittle approach of listing objects, drf-spectacular should have a list of importable namespaces that are blocked except when the exposed view as it appears in URLConf is from that namespace. rest_framework is clearly one of them, but it exposes a few views that are often include(..) into URLConf such as the login/etc. silk would be another, so it should be, so being configurable would be good.

The good news is that the previous change picked up the one good docstring which that previous issue was about, so thanks.

Another approach? Reject any docstring that would appear more than one in the API, because it is a clear indicator of bad docs , and inheritance is only one cause of bad docs.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 30, 2020

An example of a duplicate from oscarapi

  /oscarapi/login/:
    get:
      operationId: oscarapi_login_retrieve
      description: |-
        Api for logging in users.

        DELETE:
        Log the user out by destroying the session.
        Anonymous users will have their cart destroyed as well, because there is
        no way to reach it anymore

        POST(username, password):
        1. The user will be authenticated. The next steps will only be
           performed is login is successful. Logging in logged in users results in
           405.
        2. The anonymous cart will be merged with the private cart associated with
           that authenticated user.
        3. A new session will be started, this session identifies the authenticated
           user for the duration of the session, without further need for
           authentication.
        4. The new, merged cart will be associated with this session.
        5. The anonymous session will be terminated.
        6. A response will be issued containing the new session id as a header
           (only when the request contained the session header as well).

        GET (enabled in DEBUG mode only):
        Get the details of the logged in user.
        If more details are needed, use the ``OSCARAPI_USER_FIELDS`` setting to change
        the fields the ``UserSerializer`` will render.
  post:
    ...
  delete:
    ...

Obviously not something that drf-spectacular should automatically fix, and an Extension would be perfect for this, but it would be nice to have them rejected, or a warning at least.

@jayvdb jayvdb mentioned this issue Jun 30, 2020
@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 30, 2020

Another gem

@@ -6503,6 +6047,7 @@ components:
       - id
     UserAddress:
       type: object
+      description: Correctly map oscar fields to serializer fields.
       properties:
         id:
           type: integer
@@ -6569,6 +6114,7 @@ components:
       - key
     Voucher:
       type: object
+      description: Correctly map oscar fields to serializer fields.
       properties:
         name:
           type: string
@@ -6611,6 +6157,7 @@ components:
       - name
     WPartner:
       type: object
+      description: Correctly map oscar fields to serializer fields.
       properties:
         id:
           type: integer
project> grep 'Correctly map' public/schema.yaml | wc -l
20

django-oscar-api> git grep 'Correctly map'
oscarapi/serializers/utils.py:    Correctly map oscar fields to serializer fields.
oscarapi/serializers/utils.py:    Correctly map oscar fields to serializer fields.

@tfranzel
Copy link
Owner

that should resolve your first post.

regarding the second, the duplication likely happens by spectacular. the viewset level docstring is currently not elevated to the "path" object but is duplicated on the operation level. that is something achievable. however, UI wise it makes little difference because if the description is missing from the operation it is pulled from the "path" object. at least SwaggerUI handles it like that. so you got "duplication" there anyway in those cases. the only difference is that the schema in itself has a little less duplication.

@tfranzel
Copy link
Owner

as far as i'm concerned the main points of this issue are resolved. deduplication on the schema level would be another issue. wrt to the UI it makes no difference atm.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 12, 2020

fwiw, I created a post-processor which removes the "Correctly map oscar fields to serializer fields."

@tfranzel
Copy link
Owner

not sure if you noticed, but we introduced a settings explicitly for your request. all you have to do is create a new function, call the build-in function, extend the list with the oscar stuff, and set your function in the settings. best we can do as it is sometimes hard to distinguish between external and local code.

'GET_LIB_DOC_EXCLUDES': 'drf_spectacular.plumbing.get_lib_doc_excludes',

b64e915#diff-72133ec0edd940518ab4f6de60ae881dR40

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