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

Ability to disable type checking #23

Closed
wants to merge 6 commits into from
Closed

Ability to disable type checking #23

wants to merge 6 commits into from

Conversation

bpeake-illuscio
Copy link
Contributor

@bpeake-illuscio bpeake-illuscio commented Jan 19, 2019

Hello!

In my workflow I am using dacite in conjunction with Marshmallow. They way my project is set up has Marshmallow running checks on a dict against a schema before the dict is loaded, meaning the dict has been type-checked ahead of time.

It would be nice to disable dacite from running redundant type checks in this case, so I put together a pull request adding this feature. Let me know what you think, and thanks as always.

It adds an optional flag to the config class which allows the ability to disable type checks.

@konradhalas
Copy link
Owner

@bpeake-illuscio thank you for your PR!

dacite + serialization/validation library (like Marshmallow or DRF) is a way to go - I'm doing the same 👍

TBH I don't know what to think about this feature. Sure, it doesn't make sense to do the same thing twice, but still - why not? Are you worry about performance? At the end of the day, in your case, it doesn't change the result of from_dict.

@bpeake-illuscio
Copy link
Contributor Author

bpeake-illuscio commented Jan 21, 2019

Totally, It doesn't really affect performance at all.

For me it's a desire to know that all my logic for validation is in one place and that edge cases for validation will only manifest from a single library.

In essence, it saves this:

try:
    obj = schema().load(data)
except (ValidationError, dacite.WrongTypeError, dacite.UnionMatchError):
    handle_error()

And lets me worry about a single library's error-raising process when loading the object. If I run into a weird edge case that dacite does not handle, then I can simply disable the type checking.

It's less about doing the same thing twice (which in this case has totally negligible performance effects), and more about the increased fragility to having something happen two different ways.

I would posit why not allow the choice? Its a minimal change to make it available for those that want it.

@bpeake-illuscio
Copy link
Contributor Author

Hi!

I've actually run into a production instance where I now need this feature.

Consider the following:

>>> import dacite
>>> from dataclasses import dataclass
>>> from typing import TypeVar, Generic
>>> 
>>> DataType = TypeVar("DataType")
>>> 
>>> @dataclass
... class Data(Generic[DataType]):
...     value: DataType
...     
>>> class StrData(Data[str]):
...     pass
... 
>>> dacite.from_dict(StrData, {"value": "I am a string"})
Traceback (most recent call last):
    ...
TypeError: isinstance() arg 2 must be a type or tuple of types

Dacite cannot currently handle the type validation for inherited TypeVar's this way, but my solution for converting dataclasses to marshmallow schema's can. Currently, because I cannot disable type checking in dacite, I cannot ship my solution, even though I know the marshmallow half of my lib can validate the types correctly before they are loaded.

This is a good example of why I am wary of feature duplication, and would prefer to be able to disable type checking, rather than solve an edge-case twice.

I will also log this bug as an issue.

dacite.py Outdated
@@ -248,7 +251,7 @@ def _inner_from_dict_for_union(data: Any, field: Field, outer_config: Config) ->
outer_config=outer_config,
field=field,
)
elif _is_instance(t, data):
elif _is_instance(t, data) or not outer_config.validate_types:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we give up after first try? Maybe we should do something like that at the end of this function:

if not outer_config.validate_type:
    return data
raise UnionMatchError(field)

@konradhalas
Copy link
Owner

@bpeake-illuscio please check my comments :)

@bpeake-illuscio
Copy link
Contributor Author

Changes made! let me know what you think! Thanks as always!

@konradhalas
Copy link
Owner

@bpeake-illuscio manually merged in 962320a

Thank you! :)

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