-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Schemas & client libraries. #4179
Conversation
|
||
def get(self, request, *args, **kwargs): | ||
if request.accepted_renderer.format == 'corejson': |
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.
Is there any reason why this couldn't be brought into the CoreJSONRenderer
?
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 problem is less what to do with CoreJSONRenderer
and more about making sure that we preserve the behavior of that view for anything that isn't a schema renderer.
(Also I'd consider that part still slightly in flux - clearly needs refinement)
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.
making sure that we preserve the behavior of that view for anything that isn't a schema renderer
Right, my main concern with that line (which I'm sure will probably change another few times before this is merged) is that it's hard-coding corejson
as the only renderer with schema support.
Also I'd consider that part still slightly in flux - clearly needs refinement
Definitely understandable.
@@ -790,3 +791,17 @@ def render(self, data, accepted_media_type=None, renderer_context=None): | |||
"test case." % key | |||
) | |||
return encode_multipart(self.BOUNDARY, data) | |||
|
|||
|
|||
class CoreJSONRenderer(BaseRenderer): |
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.
There was a move in 3.1 to remove features which had third-party dependencies from the core and put them into third-party packages that were still highly visible. See django-rest-framework-xml, django-rest-framework-yaml, and django-rest-framework-oauth for examples.
Is that still something we are pushing for?
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.
Kinda. I see coreapi
as a foundational thing here, so it's a bit different.
The various types of schema and docs that you can use it to generate will be third party, yup.
So eg we can have third party packages for schema formats: Swagger / API Blueprint / JSON Hyperschema, for docs templates driven by a coreapi.Document object, and for various hypermedia styles.
Using coreapi
ensures that we're able to provide a common interface for all of those, so I don't have any great issues with pulling it in. If it's in core, then we're making the promise that it's an interface that is available to third-party devs, which should help drive folks making schema renderers / hypermedia renderers and docs renderers. Having said that, we could push for it to be third-party, if we wanted to, so I might be open to discussion.
|
||
router = DefaultRouter(schema_title='Server Monitoring API') | ||
|
||
The schema will be included in by the root URL, `/`, and presented to clients |
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.
"included in by" seems confusing. Maybe take out the "by"?
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.
Good catch, that's a typo.
Remaining issues have now been split out into individual tickets. (Ensure it will be released prior to DjangoCon US, and give at least one clear working day to resolve any ciritical issues, post-release) |
💯 |
Yeah. Indeed. |
@@ -127,6 +130,17 @@ def to_html(self, request, queryset, view): | |||
template = loader.get_template(self.template) | |||
return template_render(template, context) | |||
|
|||
def get_fields(self, view): | |||
filter_class = getattr(view, 'filter_class', None) |
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.
Wouldn't it make more sense to use self.get_filter_class
here?
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.
Possibly, tho we don't actually have a queryset
/model
available to us at this point, so couldn't dynamically generate the filter class if none was set explicitly.
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.
What are the problems with just calling view.get_queryset()
?
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.
Sure, that'd probably work okay.
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include: support for Django 1.10 (encode/django-rest-framework#4158 - sigh), support for schema generation (encode/django-rest-framework#4179 - required for newer versions of django-rest-swagger), and a change that prevents paginated views from re-running queries when count queries return 0 (encode/django-rest-framework#4201 - this explains the expected query count changes made in tests). LEARNER-1590
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include: - [Support for Django 1.10](encode/django-rest-framework#4158) - sigh - [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger - A [change](encode/django-rest-framework#4201) that prevents paginated views from re-running queries when count queries return 0 - this explains the expected query count changes made in tests LEARNER-1590
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include: 1. [Support for Django 1.10](encode/django-rest-framework#4158) - sigh 2. [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger 3. A change that [prevents paginated views from re-running queries](encode/django-rest-framework#4201) when count queries return 0 - this explains the expected query count changes made in tests LEARNER-1590
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include: 1. [Support for Django 1.10](encode/django-rest-framework#4158) - sigh 2. [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger 3. A change that [prevents paginated views from re-running queries](encode/django-rest-framework#4201) when count queries return 0 - this explains the expected query count changes made in tests LEARNER-1590
@tomchristie FYI, the links in your original comment at the top of this thread are broken. |
Support for schema generation & dynamic client libraries.
See the new tutorial section, schema documentation, and api client documentation.
Needed to complete tutorial 7:
--auth basic
support to Core API.Other work as part of this, prior to 3.4.0 release.
Document
,Error
andLink
in JSON renderer?ListSerializer
and other non-form.Moved to separate tickets:
SchemaGenerator
coreapi
into separate packages. (HTML, OpenAPI, HyperSchema, HAL...)Deferred:
Handle binary downloads.Not strictly required for 3.4UseNot strictly required for 3.4id
notpk
in schema representations or change serializer representations?Add to quickstartNot strictly required for 3.4