bloomberg / attrs-strict

Provides runtime validation of attributes specified in Python 'attr'-based data classes.
Apache License 2.0
52 stars 19 forks source link

Add support for Literals #65

Closed patniharshit closed 3 years ago

patniharshit commented 3 years ago

*Issue number of the reported bug or feature request: N/A

Describe your changes Adds support for Literal.

Testing performed Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned. -> See new test cases being added.

Additional context Add any other context about your contribution here.

patniharshit commented 3 years ago

@erikseulean I managed to fix failing checks (check/lint is failing on master as well) by correctly installing typing_extensions/ enum34 and also made code to work with all supported versions of Python so this is ready for a proper review.

patniharshit commented 3 years ago

As we noted above supporting everything that can come as argument in a Literal type is going to be difficult so I have for now chosen to support most common types like int, str, bool, Enum, None and Literal as valid arguments to Literal type. This is in accordance with https://www.python.org/dev/peps/pep-0586/#legal-parameters-for-literal-at-type-check-time.

  1. What else do you think should also be supported?

Also currently UnsupportedLiteralType is raised if unsupported arguments to Literal are encountered. This breaks client code during runtime if client uses attrs-strict with invalid Literal type and does not test the code. (atm this part is a seperate commit)

Instead of taking this approach we document what arguments are valid for Literal type and silently skip type checking for unsupported arguments instead of raising error.

  1. Which approach is better to deal with illegal/ unsupported arguments to Literal?
patniharshit commented 3 years ago

@erikseulean updated the documentation on behaviour of Literals and fixed failing check.

patniharshit commented 3 years ago

@gaborbernat you had pushed one commit on top my changes as "PR feedback"?

I realised I might have undoed it by force pushing without pulling your changes first.

gaborbernat commented 3 years ago

Indeed.

gaborbernat commented 3 years ago

@patniharshit I've pushed back my stuff 👍🏻 if you can do it on top of it, please 🙏🏻 thanks!

patniharshit commented 3 years ago

@erikseulean I have rebased, ready for merge.