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

Let all fields _deserialize method accept **kwargs #1007

Merged
merged 1 commit into from Oct 18, 2018

Conversation

@lafrech
Copy link
Member

commented Oct 17, 2018

Following up on #849. I think this makes things more consistent.

Adding a note to the docs saying _deserialize may receive arbitrary kwargs should allow us to add other field-specific kwargs later on without it being a breaking change.

I'm tempted to do the same to _serialize, preventively...

@lafrech lafrech added this to the 3.0 milestone Oct 17, 2018

@lafrech lafrech added the refactor label Oct 17, 2018

@lafrech lafrech requested review from deckar01 and sloria Oct 17, 2018

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Seems fine. I think doing the same to _serialize makes sense for consistency's sake. I don't feel strongly about it, though. Omitting **kwargs makes for a stricter interface, which could be good.

@deckar01

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

I thought new kwargs were a minor version change, not a major version change. Preventing old minor versions from erroring when a new kwarg features is used does not seem like a desirable behavior to me.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

So you'd rather have a strict interface where Nested and Pluck fields receive partial and no other field accepts any kwargs?

pylint complains when changing a method signature when subclassing as it is considered bad practice, I suppose.

I thought new kwargs were a minor version change, not a major version change.

It will break custom fields not accepting kwargs. Doesn't this qualify as a breaking change?

@deckar01

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

I see what you mean. I was stuck thinking about field inheritance and subclasses calling our base class methods. The issue is the marshaller calling custom fields' _deserialize method with kwargs that they don't support. That is a major change unless they are required to ignore extra kwargs. Thanks for bearing with me.

Since these methods are only expected to be called by us internally, there is no need to protect ourselves from passing a kwargs to a custom field that doesn't do anything with it. 👍

@sloria sloria merged commit 5279fef into dev Oct 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@sloria sloria deleted the dev_rework_field_signature branch Oct 18, 2018

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.