-
Notifications
You must be signed in to change notification settings - Fork 98
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
Enhance flattening #324
Enhance flattening #324
Conversation
…es care of referenced files
…polymorphic models
What's your thought process behind the removal of unused models? It seems like it requires more work, and the win is small. For me as a developer it would be surprising to not find a model in the |
The thoughts are debatable and I'm open to discussion. An example is for specs like swagger: '2.0'
info:
title: Test
version: '1.0'
paths:
/a_random_endpoint:
get:
responses:
'200':
description: 'OK'
schema:
$ref: 'https://petstore.swagger.io/v2/swagger.json#/definitions/Category' flattening such specs includes a big number of models/definitions that are never used. Why am I consider this now? Do you think that we should avoid this type of approach or revisiting it? (maybe making this behaviour configurable could be an alternative) |
Hm, yes that makes a lot of sense - we probably don't want all of the objects defined in such external specs to be included. On the other hand we should not remove any object defined in the root |
Seems fair to me, I'll try to have a look to it |
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 code seems to make sense, but it's not clear to me how your spec changes in test-data relate to it?
@sjaensch I need to make sure that we have appropriate testing before merging this. |
b2a9aa2
to
177bc6e
Compare
This PR has 2 main objectives:
$ref
or polymorphism) are available on the final flattened specsThe first point was noticed while doing some testing where specs are defined as dictionary with a reference to an external resource. Not setting the
origin_url
inbravado_core.spec.Spec
constructor causesSpec.origin_url
to be set to an empty string.Due to the fact that the empty string evaluates
False
we have the risk of not discovering models from the referenced files (as described in the documentation).An other bug is related to model name determination, during inherits discovery.
About the flattening instead the current status is that all the models in the original Spec object need to be transposed in the flattening specs and this was ensured by
add_original_models_into_known_mappings
(that has been removed).I'm updating this logic because it causes all the models defined into the referenced files of the specs to end up in the final flattening result even if the model definition is not used.
This PR replace
add_original_models_into_known_mappings
withinclude_discriminated_models
to ensure that only "useful" model definitions are propagated to the flattened specs."useful" models are models that are used by the specs:
$ref
attribute