Skip to content

Allow for custom instantiation of input serializer #228

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Allow for custom instantiation of input serializer #228

wants to merge 6 commits into from

Conversation

abdulhaq-e
Copy link

patrick91
patrick91 reviewed Jul 24, 2017
View reviewed changes
docs/rest-framework.rst Outdated
class MySecondAwesomeMutation(SerializerMutation):

@classmethod
def instantiate_serializer(cls, instance, args, request, info):
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this get_serializer, @syrusakbary what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I just had a quick look at drf source, it's indeed get_serializer everywhere. I'll make the change.

@patrick91
Copy link
Member

Looks good @abdulhaq-e, I've add just a comment :)
also let's wait for @syrusakbary feedback, since I'm not a maintainer on this project

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 92.268% when pulling 412747d on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 92.268% when pulling 412747d on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 92.268% when pulling 412747d on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@coveralls
Copy link

coveralls commented Jul 24, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.09%) to 92.268% when pulling 412747d on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@patrick91
Copy link
Member

also, can you add a test? :)

@coveralls
Copy link

coveralls commented Jul 24, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.09%) to 92.268% when pulling 9240fbe on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@abdulhaq-e abdulhaq-e force-pushed the custom_instantiation branch from 9240fbe to b5a32be Compare July 24, 2017 15:13
@coveralls
Copy link

coveralls commented Jul 24, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.09%) to 92.268% when pulling b5a32be on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@abdulhaq-e abdulhaq-e force-pushed the custom_instantiation branch from b5a32be to bc7d96b Compare July 24, 2017 16:31
@coveralls
Copy link

coveralls commented Jul 24, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.09%) to 92.268% when pulling bc7d96b on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@coveralls
Copy link

coveralls commented Jul 24, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 92.79% when pulling c5dd913 on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@abdulhaq-e
Copy link
Author

I added the tests.

@patrick91 While writing the tests, I may have discovered a bug; serializers with no fields cannot be defined. If you see the test file and look at MyBasicSerializer, the field whatever cannot be deleted or else the tests will fail with:

AssertionError: MyBasicSerializerInput fields must be a mapping (dict / OrderedDict) with field names as keys or a function which returns such a mapping.

I know it's an edge case but it maybe something to think about.

patrick91
patrick91 reviewed Jul 24, 2017
View reviewed changes
graphene_django/rest_framework/tests/test_mutation.py Outdated
@@ -68,3 +69,47 @@ class Meta:
model_input_type = model_input._type.of_type
assert issubclass(model_input_type, InputObjectType)
assert 'cool_name' in model_input_type._meta.fields


def test_custom_serializer(capfd):
Copy link
Member

Choose a reason for hiding this comment

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

what's capfd here?

Copy link
Member

Choose a reason for hiding this comment

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

oh, got it: https://docs.pytest.org/en/latest/capture.html#accessing-captured-output-from-a-test-function (TIL) :)

I think you can remove it, since it not used :)

Copy link
Author

Choose a reason for hiding this comment

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

Removed

@patrick91
Copy link
Member

@abdulhaq-e well spotted, not sure if serialiser without fields are a thing, do you know any use case for it?

@coveralls
Copy link

coveralls commented Jul 27, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 92.79% when pulling a755903 on abdulhaq-e:custom_instantiation into cec1a84 on graphql-python:master.

@abdulhaq-e
Copy link
Author

serialiser without fields are a thing, do you know any use case for it?

Their create method can be used to run something which may only use the serializer context for example. Even if it's an edge case, the error message is not accurate. I'll try to have a look.

@syrusakbary syrusakbary force-pushed the master branch 2 times, most recently from bdc7189 to f93251b Compare September 1, 2017 08:12
@glopezmartin glopezmartin mentioned this pull request Oct 9, 2017
Closed
@spockNinja
Copy link
Contributor

@abdulhaq-e

I appreciate the flexibility this change will provide. It will cover use-cases like #285.

It looks like the new test is not passing in the travis build. Please see https://travis-ci.org/graphql-python/graphene-django/jobs/271085453#L709 for details.

Also, as to your comments about the edge-case. I think that problem should be resolved in a separate PR. Feel free to file an issue with the use-case or open up another PR for that problem.

Thanks for your work! It is very much appreciated. 😄

@firaskafri
Copy link
Collaborator

@abdulhaq-e still interested to work on this?

@abdulhaq-e
Copy link
Author

@firaskafri I have a lot in my hand right now so unfortunately I can't :(

@phalt
Copy link
Contributor

phalt commented May 3, 2019

I'd like to tidy up some of the PRs, so if we still want to work on this let me know or I will close this in 1 week.

@phalt phalt closed this May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrick91 patrick91 patrick91 left review comments

@spockNinja spockNinja spockNinja approved these changes

Assignees
No one assigned
Labels
changes requested ✨enhancement needs test case
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@abdulhaq-e @patrick91 @coveralls @spockNinja @firaskafri @phalt @syrusakbary