lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
384 stars 46 forks source link

Literal types no longer cause a warning on decoding #221

Closed norabelrose closed 1 year ago

norabelrose commented 1 year ago

On master, you can't deserialize a dataclass with Literal type annotations without running into warnings like this:

UserWarning: Unable to find a decoding function for the annotation typing.Literal['first', 'last', 'mean'] (of type <class 'typing._LiteralGenericAlias'>). Will try to use the type as a constructor. Consider registering a decoding function using `register_decoding_fn`, or posting an issue on GitHub.

I've fixed this by adding an extra if-block in helpers/serialization/decoders.py that checks for Literal types:

if is_literal(t):
        logger.debug(f"Decoding a Literal field: {t}")
        possible_vals = get_type_arguments(t)
        return decode_literal(*possible_vals)

The Callables created by the decode_literal function simply check if the input values are among those permitted by the Literal type, and raise a TypeError if so.

Please let me know if there's anything else I need to do (e.g. add a test?) to make this PR mergeable.

lebrice commented 1 year ago

Hey there @norabelrose, thanks for making this!

You're right that this would ideally require a test. I'll get back to you with an example of what that could look like, but in the meantime, you could probably take a look at this:

The goal of the tests would be to:

  1. reproduce that there is indeed raising a warning without the fix
  2. that the fix removes it
  3. check that the decoding works as expected.

If you feel like taking a swing at this, let me know. If not, no worries! :) Let me know what you think.

norabelrose commented 1 year ago

All right let me take a look at the tests

norabelrose commented 1 year ago

Hmmm I'm actually not able to replicate the bug anymore? Not sure what's going on

norabelrose commented 1 year ago

Nvm now I'm seeing the bug again

norabelrose commented 1 year ago

Just added a test; should fix #226