genomoncology / related

Nested Object Models in Python with dictionary, YAML, and JSON transformation support
MIT License
198 stars 15 forks source link

Urlfield before Stringfield produces an error #20

Closed ChristianSauer closed 5 years ago

ChristianSauer commented 5 years ago

Just as a quick bug report: This does not work:

@related.immutable
class ImageOptions(object):
    registry = related.URLField()
    email = related.StringField()

ValueError: No mandatory attributes allowed after an attribute with a default value or factory. Attribute in question: Attribute(name='email', default=NOTHING, validator=<instance_of validator for type (<class 'str'>,)>, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({'key': None}), type=None, converter=<function str_if_not_none at 0x000001C58DFC6158>)

This does work:

@related.immutable
class ImageOptions(object):
    email = related.StringField()
    registry = related.URLField()

Rather surprising that the order of arguments matter

imaurer commented 5 years ago

Looks like URLField has a default required value of false, as does UUIDField. All other fields are required equals true by default. Need to review whether I did that intentionally for some reason. If not, I will correct it. Thanks for the heads up.

imaurer commented 5 years ago

Also, if you are curious about the root cause, it’s a “feature” of the underlying attrs library that supports positional arguments to classes.

https://github.com/python-attrs/attrs/issues/38

GeorgeLubaretsi commented 5 years ago

@imaurer What are the consequences of making all fields to pass kw_only=True?

GeorgeLubaretsi commented 5 years ago

Looks like kw_only is only supported in Python 3+ https://github.com/python-attrs/attrs/commit/123df6704176d1981cf0d8f15a5021f4e2ce01ed

imaurer commented 5 years ago

The URL field no longer behaves in this arbitrarily restrictive manner: https://github.com/genomoncology/related/blob/master/tests/issues/test_issue_020.py

Thanks @GeorgeLubaretsi for the PR. That's a bigger change that requires more deliberation.