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

Schema autogeneration #4329

Closed
wants to merge 5 commits into from

Conversation

cecedille1
Copy link

Schema Autogeneration

This pull request addresses Schema autogeneration from the API views.

  1. it resolves a bug where the higher namespace urls (/account/{account_id}/books/{pk}/) are hidden by lower namespace (/account/{pk}/).
  2. It uses the docstring of the method that will handle the request (either get, post, put, etc for the APIView or list, retrieve, update for the viewset) as the description of the coreapi.Link.
  3. It uses the help_text of the serializers fields as the description of the coreapi.Field.
  4. It capitalize the names of the Link.

Grégoire ROCHER added 5 commits July 28, 2016 15:16
For viewset, it exports the corresponding action (list, retrieve,
update, etc.). For simple APIView, it returns the http handler (get,
post, put, etc.).
Before, an API plugged to a namespace (for instance /account/{pk}/)
would erase API plugged to the same namespace with additional URL
segments: (for instance /account/{user_id}/book/{pk}/)
@tomchristie
Copy link
Member

Great stuff in here, thanks!
Will review shortly.

@tomchristie
Copy link
Member

tomchristie commented Aug 11, 2016

Okay, so there's a number of different aspects here, some we want, some we probably don't, some may need further thought.

  1. Yes, we should resolve that, although not 100% clear to me exactly what we should use for the key generation. Needs further thought.
  2. Useful, although note that this would also pull in unhelpful docstrings from the default mixin implementations, so not sure how we should best handle that.
  3. Yes.
  4. Not totally convinced if we should be using Humanized or programatic formatting for the keys.

The path_descriptions API you've added is also perfectly reasonable, tho again, I'm not 100% sure if it's exactly what we want or not.

@kevin-brown
Copy link
Member

  1. Sounds like an issue, but I can't think of a clean way to resolve them properly. Someone else who has more experience reverse-engineering url patterns can probably chip in here.
  2. I'm a fan of this, because I personally have a tendency to write method-specific documentation when I'm overriding them. With that said, I do see the issue that Tom mentioned with the default mixins having developer-oriented documentation instead of user-oriented documentation. I'd personally prefer developer-oriented documentation there, so I don't have a solution.
  3. Handled in Add form field descriptions to schemas #4387.
  4. Imo, this is better handled client-side. The keys appear to match the url path for links right now, and this change can be done on the client if needed.

@tomchristie
Copy link
Member

On second thoughts I noted that the .get etc... methods that we provide in REST framework don't have default docstrings, so we could use them. Although some users will be using base classes and/or mixins where the docstrings may not be correct/relevant, so point still applies, but less so than it would otherwise.

@tomchristie
Copy link
Member

  • Some remaining work on (1) prior to the 3.4.4 release - still not handling that quite right.
  • For (2) I'm happy to reassess that as part of 3.5.0 - happy to see an individual issue opened for it. We're adding in the form field descriptions now. We could also look at addressing path field and query field descriptions.
    1. Now resolved.
    1. Going to stay with lowercase for now. We'll firm up the generator API methods and document them as public API, so that folks can introduce third party packages with overrides if wanted.

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.

3 participants