Let data_key and attribute default to field name

[lafrech]

Closes #1864.

Trying to address #1864, I get typing issues, as foreseen in https://github.com/marshmallow-code/marshmallow/issues/1864#issuecomment-909225360. I wouldn't mind a bit of help, here.

This change spares users the hassle of checking if data_key is None, as can be seen in the changes in schema.py. This would be used in apispec, for instance.

Notes:

• In Pluck, we don't check for None but only for truthyness, which may be wrong for empty strings. I didn't look into it any further but this PR would fix it.
• I also intend to add data_key to the repr. The printed name will be the actual value, be it the parameter or the field name (default). I don't think it is an issue.
• I also intend to do the same with attribute, symmetrically.

[deckar01]

error: Argument 1 to "get" of "Mapping" has incompatible type "Optional[str]"; expected "str"

The problem is that Fields.__init__ assigned a Optional[str] type to data_key. _bind_to_schema guarantees that field_obj.data_key is a str, but the type checker doesn't know that _deserialize is guaranteed to run after _bind_to_schema. I guess the get type annotation enforces this argument, but AFAICT, None doesn't actually error. 🤷

It wants you to ensure data_key is never None. The only thing I can think of is to make data_key a @property that raises an exception if the field is unbound.

[lafrech]

Yes, we agree on the reason behind the error.

It would be a shame to "complicate" the code, even slightly, to please the linter.

I suppose we could add comments to skip test on each failing line, but I'd rather find a way to centralize that by somehow forcing the type of self.data_key once and for all.

We could use a cast:

self.data_key = typing.cast(str, data_key)

but doing this in __init__ before field binding would be wrong.

Or we do it just before the type check error:

field_name = typing.cast(str, field_obj.data_key)

Still not ideal because:

• It is not done everywhere, only where the error was triggered. Other errors may occur later here or in other libs.
• It is executed on each deserialization, although IIUC it is neglectable.

I just pushed a commit doing that.

[lafrech]

Notes:

• I renamed a few local variables in Schema methods to be more explicit: data_key instead of field_name. We might want to rename field_name attribute of _call_and_store because it not necessarily the field name. That can be done later.

• In Field.get_value, I had to keep the if self.attribute is None check because the field can be used while not bound to a schema.

[lafrech]

I performed a quick test on apispec codebase to see how things could be simplified there and the result is a bit disappointing. apispec operates on schema instances or classes, and when operating on a class, fields are not bound to the schema so we can't rely on data_key or attribute being set.

This feature would be more useful if it occurred on schema class creation, not instantiation. Technically, that could mean adding a _set_name method to Field that would be called from Schema._get_fields. This is what I do in the last commit. Although setting name at schema creation seems more logical to me, the downside of this is that custom fields, especially container fields, may now have to override both _bind_to_schema and _set_name. (Part of the trouble there is related to inferred or Meta.include fields, which may be dropped in marshmallow 4 (at least inferred ones: #1356).)

With this last commit, the changes in apispec work.

Notes:

• The change in apispec is minimal (2 lines changed), so the benefit of this is rather limited.)
• If I understand @kasium correctly in #1864, they are using _declared_fields so they are working on Schema classes, not instances, therefore, they fall in the same case as apispec and the change would only be beneficial if done at Schema creation.

[kasium]

@lafrech yes, that's right. I use the declared_fields attribute. But I guess using a schema instance would not add too much trouble if somehow cached (not your worry). However, having it on class level would still be nice