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

New undefined parameter action: Warn #212

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

RunOrVeith
Copy link
Contributor

This pull requests adds a 4th possible Undefined action: Undefined.WARN.

When using this action, a RuntimeWarning is issued for undefined parameters,
otherwise it behaves the same as Undefined.EXCLUDE.

Because marshmallow does not have this option available, we default back to EXCLUDE when using schema().

I updated the README to include:

  1. You can issue a RuntimeWarning for undefined parameters by setting the undefined keyword to Undefined.WARN (WARN as a case-insensitive string works as well). Note that you will not be able to retrieve them using to_dict:

    from dataclasses_json import Undefined
    
    @dataclass_json(undefined=Undefined.WARN)
    @dataclass()
    class WarnAPIDump:
        endpoint: str
        data: Dict[str, Any]
    
    dump = WarnAPIDump.from_dict(dump_dict)  # WarnAPIDump(endpoint='some_api_endpoint', data={'foo': 1, 'bar': '2'})
    # RuntimeWarning("Received undefined initialization arguments (, {'undefined_field_name': [1, 2, 3]})")
    dump.to_dict()  # {"endpoint": "some_api_endpoint", "data": {"foo": 1, "bar": "2"}}

I've also cleaned up the test cases for undefined parameters a bit (grouped them by the action that they test and gave them more descriptive names). So don't worry if the diff for the test case file is large, I've just moved stuff around more or less and added tests for WARN.
@lidatong

@RunOrVeith
Copy link
Contributor Author

@lidatong

@lidatong lidatong self-requested a review June 5, 2020 19:21
@lidatong
Copy link
Owner

lidatong commented Jun 5, 2020

Thanks for implementing this and submitting another PR!

I've been thinking about this one. I think this functionality is useful and different flavors of it have been requested in other issues / PRs (#223, #207, maybe others).

However, I'm not sure this should be nested under the existing the Undefined functionality. In terms of concerns, they seem like two separate things, which I think is illustrated by this block in your PR:

                if undefined_parameter_action == Undefined.WARN:
                    # mm has no warn implementation, so we resolve to ignore
                    undefined_parameter_action = Undefined.EXCLUDE

In addition to it no longer mirroring marshmallow's parameters, I think it's worth considering whether it couples two unrelated things:
one is fundamentally core logic ("I've delegated this field to put all the extra stuff") vs. the other which is administrative ("warn me when I get unused fields").

Rather, I think this functionality should be part of a unified API for configuring warnings / validations / etc.

In fact, the specification of this configuration may not even be an individual class-level thing (i.e. here it's specified as a kwarg to the class decorator). As a user, I imagine I would generally want warnings either on or off fat a global-level. Perhaps I might even dynamically switch them on when I'm using a certain external API, then off.

There's a potential argument to add Unknown.WARN to the public API specifically, just to "get the functionality in there for now". However, I'm becoming increasingly wary of what gets added to the public API -- I was originally very open to new features / experimental stuff since this library is still pre-1.0 for the sake of functionality and my plan was to "clean up" or "re-design" on a 1.0 release -- but as more people depend on this library I'm increasingly wary of breaking backwards compatibility

That said, I think a lot of the code in your PR can be repurposed to be configured by the global config.

Let me know what you think, and just want to say I appreciate your contributions again!

@RunOrVeith
Copy link
Contributor Author

I see your concerns, and I want to make an argument and a suggestion how to get both things:
For me at least, the Undefined enum would be the first point to look how I can influence what happens when undefined parameters are encountered.
Furthermore, we use dataclasses_json in a way where we want the explicit control for each class, e.g. deserializing API responses from APIs that we control (we want to raise an error if we find undefined parameters here) vs an external, frequently changing API response where we only care about a few arguments that are consistent. In that case we would like to just Issue a warning.
Your argument that warnings are more administrative is understandable, but then the same goes for errors, which are already in the undefined logic.

In order to get fine grained control as well as be able to set in on the global level, I'd suggest to use something like default_undefined_action inside the cfg object, that is then used instead of a hard-coded default when nothing is supplied.
That way you can make it e.g. always raise an error, but you can still overwrite it with custom behavior on a per-class basis.

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

Successfully merging this pull request may close these issues.

2 participants