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

Pass current state of the definition to definition_helpers in APISpec.definition #122

Merged
merged 2 commits into from Mar 19, 2017

Conversation

@martinlatrille
Copy link
Contributor

commented Mar 17, 2017

Now pass the current state of the definition (i.e. ret) as a kwarg to all definition helpers.

It will allow helpers to do modifications of nested value in the definition (like modifying a schema's property for example) without overriding the whole top-level dict (properties, in our example).

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

Looks good.

Note that this relies on the order in which the helpers are executed, which is the registration order: the order of the plugins in the list you pass at APISpec init. This is also the case for path helpers. This could be an issue if you need a different order for path helpers and definition helpers. But this is more of a theoretical issue, I don't think it would be blocking in practice. Anyway, there's no reason to block this PR until this is addressed.

Care to add a test before we can merge this?

Also, I suggest this change:

- Definition: the current state of the definition of the schema
+ definition (dict): current state of the definition
@sloria
Copy link
Member

left a comment

Once the test and small doc change have been made, this is good to merge. Thanks for the PR, and thanks @lafrech for reviewing.

@@ -269,6 +269,8 @@ def register_definition_helper(self, func):
- Return a `dict` representation of the definition's Schema object.
The helper may define any named arguments after the `name` argument.
In `kwargs`, you will find among other things:

This comment has been minimized.

Copy link
@sloria

sloria Mar 17, 2017

Member

To piggy-back on @lafrech 's suggestion, how about:

``kwargs`` will include (among other things):
- definition (dict): current state of the definition

This comment has been minimized.

Copy link
@martinlatrille

martinlatrille Mar 17, 2017

Author Contributor

Done!

@martinlatrille

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2017

Thanks for the review guys, I've edited the doc and added a test!

@sloria

sloria approved these changes Mar 19, 2017

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 19, 2017

Looks good. Thanks for making those changes.

@sloria sloria merged commit a5c72ae into marshmallow-code:dev Mar 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.