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

Field: respect single responsability principle, don't access parent object #1046

Open
lafrech opened this issue Nov 12, 2018 · 2 comments
Open

Comments

@lafrech
Copy link
Member

lafrech commented Nov 12, 2018

I have the feeling we could simplify Field._serialize signature from _serialize(self, value, attr, obj, **kwargs) to _serialize(self, value).

From a quick search, obj is used in Nested to populate the ValidationError. I need to get my head around this. Shouldn't it be nested_obj?

It is also used in FormattedString. This field achieves a function that could be achieved otherwise (property, pre_dump,...). I think it could be dropped without too much collateral damage.

There may be an issue with Function, too.

I didn't see where attr and kwargs are used.

It boils down to whether a field should have access to the whole structure or only its own value.

This is more or less related to #799 and probably #900/#940.

Edit: Marking as 3.0 because it is a breaking change, but it could be postponed, just like #799. If my intuition is correct, it could make for a simpler and saner interface, forbidding hard-to-debug corner cases.

@lafrech
Copy link
Member Author

lafrech commented Nov 16, 2018

I've been giving this a bit of thought.

**kwargs is something I added myself a few weeks ago in #1011. Goldfish memory... Scratch that.

attr and obj look like unnecessary complexity (and API exposure) when dealing with normal fields. The only fields that use them are FormattedString, Function and Method.

It wasn't clear to me straight away, just an intuition, but the question is about the responsibility of the field. Hence my link to #799.

@deckar01 wrote:

I think the underlying issue here is that Field is being given the responsibility of accessing schema level objects and plucking its own data out. That seems like it should be the responsibility of the schema or the marshaller.

I agree with this. It conflicts with aforementioned fields and with #900/#940.

We could drop these fields and state that those use case should be covered by other means.

Those means can be properties in the model, but some may argue this is the responsibility of the serialization layer.

Or it can be done in pre/post_dump/load decorators, but if we want to rely on them for this, we'll probably want to enforce the order of the decorators (#600).

We could postpone this and #799 to marshmallow 4.x.

@lafrech lafrech changed the title Field._serialize: only pass value, don't give access to the whole object Field: respect single responsability principle, don't access parent object Nov 16, 2018
@sloria sloria modified the milestones: 3.0, 4.0 Dec 27, 2018
@lafrech
Copy link
Member Author

lafrech commented Jun 4, 2019

Remove root from Nested.

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

No branches or pull requests

2 participants