Skip to content
This repository has been archived by the owner on Oct 10, 2024. It is now read-only.

use polyfield for union #2

Closed
wants to merge 3 commits into from
Closed

use polyfield for union #2

wants to merge 3 commits into from

Conversation

CedricCabessa
Copy link

marshmallow-union is not supported anymore and have some known issues
see:
lovasoa#67

Author advise to switch to marshmallow-polyfield

marshmallow_dataclass/__init__.py Outdated Show resolved Hide resolved
marshmallow_dataclass/__init__.py Outdated Show resolved Hide resolved
tests/test_field_for_schema.py Show resolved Hide resolved
candidate = SchemaPolyfieldProxy(dclass)
candidate.check_deserialization(obj_dict)
return candidate
except Exception:
Copy link

Choose a reason for hiding this comment

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

can this be more precise? like (TypeError, ValidationError) (if indeed .dump and .load raise ValidationErrors)

Copy link
Author

Choose a reason for hiding this comment

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

we try every schema, and each of them can trigger an error (we pick the first one that "works")

For example, when we give a string to int schema we get ValueError: invalid literal for int() with base 10: 'hello' or if we give an int to a dict schema AttributeError: 'int' object has no attribute 'keys'

I suppose custom schema can trigger any type of error

Copy link

Choose a reason for hiding this comment

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

ah I see, ok makes sense

Comment on lines 54 to 55
raise marshmallow.exceptions.ValidationError(
"cannot deserialize")
Copy link

Choose a reason for hiding this comment

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

So far, the validation errors are quite specific in the rest of package. Should we make this text a join of the different errors that were raised above? Otherwise they are swallowed.

But maybe it would make this error hard to read, not sure..

Copy link
Author

Choose a reason for hiding this comment

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

done

@CedricCabessa CedricCabessa force-pushed the polyfield branch 3 times, most recently from 1501a98 to 34e95b7 Compare May 13, 2020 12:39
marshmallow-union is not supported anymore and have some known issues
see:
lovasoa#67

Author advise to switch to marshmallow-polyfield
Copy link

@YBadiss YBadiss left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's propose that upstream!

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

Successfully merging this pull request may close these issues.

3 participants