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

Unnecessary call to `get_value` for `Function`, `Method` fields #208

Closed
jmcarp opened this Issue May 4, 2015 · 1 comment

Comments

Projects
None yet
2 participants
@jmcarp
Contributor

jmcarp commented May 4, 2015

Related to #207.

On serializing, Function and Method fields make a call to Field#get_value. The result of this call is then discarded, because the _serialize method of both fields ignores the value parameter that gets passed in. In most use cases, this won't have any meaningful performance penalty, but it's still unnecessary work. And incidentally, the issue I ran into in #207 wouldn't have come up if Field#serialize could skip the call to Field#get_value where that value won't be used.

How about skipping the call to get_value when _CHECK_ATTRIBUTE is False? It seems like that would be more consistent with the naming of the flag than the current behavior, and it would save the extra call.

@sloria

This comment has been minimized.

Member

sloria commented May 4, 2015

+1 to removing the unnecessary call when _CHECK_ATTRIBUTE = False. In that case, we can just pass None or missing as the value argument.

@sloria sloria added this to the 2.0.0 (final) milestone May 4, 2015

jmcarp added a commit to jmcarp/marshmallow that referenced this issue May 5, 2015

Skip call to `get_value` when not `_CHECK_ATTRIBUTE`.
For `Field` subclasses with `_CHECK_ATTRIBUTE` set to `False`, the call
to `Field#get_value` in `Field#serialize` is unnecessary because its
return value is never used. This patch skips the call to
`Field#get_value` in this case.

[Resolves marshmallow-code#208]

@sloria sloria closed this in #210 May 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment