Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

Don't return parent's child_document_resources in subclasses. #87

Merged
merged 3 commits into from Aug 11, 2016

Conversation

thomasst
Copy link
Member

@thomasst thomasst commented Aug 5, 2016

By default, don't inherit child_document_resources. This lets us have multiple resources for a child document without having to reset the child_document_resources property in the subclass.

Consider the following example:

class ParentResource(Resource):
    child_document_resources = { Child: 'ChildResource' }

class ChildResource(ParentResource):
    document = Child
    fields = ['id']

class VerboseChildResource(ChildResource):
    fields = ['id', 'foo', 'bar']

If we call VerboseChildResource().serialize(obj), before this PR it would delegate the call to ChildResource since VerboseChildResource would inherit child_document_resources from ParentResource. This is usually not expected behavior, so I'm changing get_child_document_resources to only return the current class' child_document_resources. In the rare case where this isn't desirable, a parent can always explicitly change the behavior by overriding get_child_document_resources.

@thomasst thomasst self-assigned this Aug 5, 2016
@thomasst thomasst force-pushed the dont-inherit-child-resources branch from 4c2828f to 2dbec4c Compare August 8, 2016 18:56
@thomasst
Copy link
Member Author

thomasst commented Aug 8, 2016

@wojcikstefan or @tsx please review.

@thomasst
Copy link
Member Author

thomasst commented Aug 9, 2016

@closeio/engineering reminder to review

@@ -3,7 +3,7 @@
# Stops exit traceback on tests
# TODO this makes flake8's F401 fail - maybe there's a better way
try:
import multiprocessing
import multiprocessing # noqa
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does # flake8: noqa work here? If so, I think it's better because it's more obvious what it pertains to

@wojcikstefan
Copy link
Member

LGTM

@thomasst thomasst merged commit 61eac3b into master Aug 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants