OpenAPI3 proposal: Components #7096
Conversation
|
Super @tfranzel-cashlink. Thanks for the quick follow-up. (Just to set your expectations, I shall review this over the holidays and we'll look to have it ≈all in after the new year.) |
|
Sure thing. No problem. As long as we end up in a state where the generation is easily extendable/adaptable, i'm absolutely fine with ≈all. Happy Holidays! |
|
Tried this PR today. Cool that with this you can use However for me it did break on https://github.com/encode/django-rest-framework/pull/7096/files#diff-16b3da8ab434dab83a9fcf30f1322643R266. When I locally added the method argument it seemed to work fine! |
|
@jgadelange good catch. that one got lost when i converted the PR. should work now as expected |
- minimal subset of features from PR #7089 - adapted tests
|
Right, picking this up to review this week. (Happy New Year all 🎊) |
carltongibson
left a comment
carltongibson
left a comment
There was a problem hiding this comment.
OK, so @tfranzel-cashlink. Can I ask you to further break this down into smaller changes (I've indicated some places) that we can review, document if necessary and test separately?
The main "Components" change here is interesting. I need to think about it more. I'm no immediately happy about injecting the registry class into the inspector instance: the generator should just be able to ask if for what it needs… But it'll be easier to think about as a smaller isolated change.
| def add_arguments(self, parser): | ||
| parser.add_argument('--title', dest="title", default='', type=str) | ||
| parser.add_argument('--url', dest="url", default=None, type=str) | ||
| parser.add_argument('--api-version', dest="api_version", default=None, type=str) |
There was a problem hiding this comment.
Separate PR. Tests & Docs.
| mapped = self._map_serializer(method, serializer) | ||
| # empty serializer - usually a transactional serializer. | ||
| # no need to put it explicitly in the spec | ||
| if not mapped['properties']: |
There was a problem hiding this comment.
This block could go in a separate PR, with tests. ("Needing to skip empty serializers" is a separate issue.)
| } | ||
| return {'200': self._get_response_for_code(path, method, schema)} | ||
|
|
||
| def _get_response_for_code(self, path, method, serializer): |
There was a problem hiding this comment.
This too seems like a separate change.
|
Thank you Carlton for taking the time. Could you please clarify what exactly is not to your liking? Or can you suggest a different approach to adding components? I don't yet understand the direction in which you would like to take this. Honestly, I didn't see a lot of options doing this without completely refactoring the code (and breaking compatibilty with coreapi). The pure functional nature of the generator, the usage of the descriptor protocol, and the sibling implementation in CoreAPI severely limit the options on how to achieve this. I would agree that it is not the prettiest of approaches. However, for doing components there needs to be a persistent state kept somewhere somehow. I would like to nail down how you would like to have that first, because almost all of the remaining changes hinge on that. |
|
We’re not maintaining CoreAPI anymore. Don’t worry about that. (They’re already not equivalent.) I have in mind something like asking the inspector for the the component ID, and then doing the full mapping if it's new, rather than passing the whole registry. But as I say, I still need to think that through fully. First I’d very much like to separate the different features and fixes into separate PRs, that are each properly docd and tested. (For instance the CLI options are small and we can hopefully just merge them.) |
|
Closing in favour of #7124. Thanks for the input! |
2 participants
As requested, this is a subset of PR #7089, because it contained too much changes.
Thanks @carltongibson! Sorry that this is still a big change, as the code structure didn't easily accommodate a persistence layer. The remaining additions should be a lot more clearer after that though.
Feature subset here:
PATCHserializers (no required fields)generateschemafile output and more parameters for generateschemagenerateschemabugfix yaml aliases output in schema