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

Handling of None when assigned to non-optional attribute #655

Open
ex-nerd opened this issue Jun 26, 2020 · 10 comments
Open

Handling of None when assigned to non-optional attribute #655

ex-nerd opened this issue Jun 26, 2020 · 10 comments
Labels

Comments

@ex-nerd
Copy link

ex-nerd commented Jun 26, 2020

It would be nice to have control over what happens when someone attempts to set a non-optional attribute values explicitly to None: set default value, or raise ValueError.

The following example shows a rough example, including a possible use case dealing with deserialized data that might have inappropriate or missing values (yes, I'm aware of cattrs but that would require constructing a full object rather than simply applying partial updates).

import attr

@attr.s()
class Test:
    i: int = attr.ib(default=3)
    l: list = attr.ib(factory=list)

t1 = Test(i=None, l=None)
print(repr(t1))
# Actual: Test(i=None, l=None)
# Desired: Test(i=3, l=[])

input = {"i":None}
t2 = Test(i=5, l=[1,2,3])
t2.i = input.get('i')
t2.l = input.get('l')
print(repr(t2))
# Actual: Test(i=None, l=None)
# Desired: Test(i=3, l=[])

While I'm aware there may be some concerns about modifying existing behavior (and thus unintentionally breaking code and existing workarounds), these behaviors could initially be enabled via flags on attr.ib, e.g. attr.ib(default=3, default_if_none=True) or attr.ib(default=3, error_if_none=True).

This is sort of the inverse of #460

@hynek
Copy link
Member

hynek commented Jun 27, 2020

Sounds like a case for a validator to me?

@ex-nerd
Copy link
Author

ex-nerd commented Jun 27, 2020

@hynek That would solve one of the use cases but validators can't change the value, can they? And if so, wouldn't have access to what the default value should be.

@hynek
Copy link
Member

hynek commented Jun 29, 2020

There's attr.converters.default_if_none then. Converters run before validators, so you should be able to emulate your desired behavior.

@ex-nerd
Copy link
Author

ex-nerd commented Jun 29, 2020

@hynek That's exactly what I'm looking for. Not sure how I missed that in the docs, thanks!

@ex-nerd ex-nerd closed this as completed Jun 29, 2020
@ex-nerd
Copy link
Author

ex-nerd commented Jun 29, 2020

Wait, I misread the description on that converter, and there are some issues with it. While that is the behavior I'm suggesting here, use of the default_if_none converter has some issues:

  • converters don't have access to the model, so using this will require specifying the default value twice
    • this leads to potential code maintainability problems because users will occasionally/accidentally only update one value
    • if you don't also include the default parameter for the attribute, the below example would result in a TypeError: __init__() missing 1 required positional argument: 'i'
  • doesn't actually work when explicitly setting a value to None

See:

import attr
from attr.converters import default_if_none

@attr.s()
class Test:
    i: str = attr.ib(default="init",converter=default_if_none(default="converter"))

# Sets via init: Test(i='init')
print(repr(Test()))

# Sets via converter: Test(i='converter')
print(repr(Test(i=None)))

# Doesn't trigger converter: Test(i=None)
t = Test(i="original")
t.i = None
print(repr(t))

@ex-nerd ex-nerd reopened this Jun 29, 2020
@wsanchez
Copy link

The t.i = None case can be caught now with the on_setattr hook.

@zhiltsov-max
Copy link

zhiltsov-max commented Jul 23, 2021

Hi, we have tried to utilize default_if_none and found it is a bit verbose and clumsy - it does not have access to the attribute definition, so we might need to pass other converter / factory / type twice to use them with this converter. We workarounded it with a validator, because they can everything converters can, but also have access to attribute definitions.
The implementation is here and the code using it is here (multiple occurrences across the file).

Shortly, the ideal solution I see is a way to specify default / factory / type once, an independent converter or validator, and a default_if_none thing, which uses what is defined (uses default, calls factory or type c-tor if none is passed, or calls the converter or type c-tor on the value passed).

Maybe #709 could solve this.

@hynek
Copy link
Member

hynek commented Jul 28, 2021

I would love having a more coherent story here, but what's wrong with:

In [1]: import attr

In [2]: @attr.define
   ...: class C:
   ...:     x: int = attr.field(default=None, converter=attr.converters.default_if_none(42))
   ...:

In [3]: C()
Out[3]: C(x=42)

In [4]: C(x=None)
Out[4]: C(x=42)

@zhiltsov-max
Copy link

zhiltsov-max commented Jul 28, 2021

It is a bit hard to recall all the details after a year, but I'll try 😄. I'm pretty sure I tried to simulate the C++ struct member definition behaviour.

Let's say we declare a field and want it to always have a certain non-trivial type. We want to convert any other input to this type (maybe avoid copying if the input is of the right type), and also want to use a default constructor, if no value is provided:

@attr.define
class C:
    x : list = attr.field(factory=list, converter=attr.converters.default_if_none(factory=list))


>>> C()
C(x=[])
>>> C(None)
C(x=[])
>>> C({4})
C(x={4})

Here we're specifying the type 3 times, and the default value is specified twice. We haven't specified our conversion function yet, so let's do this:

@attr.define
class C2:
    x : list = attr.field(factory=list, converter=attr.converters.pipe(
        attr.converters.default_if_none(factory=list),
        list # or lambda x: isinstance(x, list): x else list(x)
             #                            ^^ in a generic function we could use the attribute definition 
             #                               that's why 3-arg converters could be useful
    ))


>>> C2()
C2(x=[])
>>> C2(None)
C2(x=[])
>>> C2({4})
C2(x=[4])

As you can see, there is a lot of repetition, but the example is quite simple and basic.

Maybe, we could just write:

@attr.define
class C:
    x : list # maybe with = [] (which is wrong, but anyway) or a factory
    y: Optional[list] # allows None

@hynek
Copy link
Member

hynek commented Jul 29, 2021

Hmm, I'm afraid you're asking for too much, your example looks good for simple cases but falls apart/becomes too unpredictable when you think it further – especially with more complex type annotations. Case in point, the fact that list is both a type and a factory is coincidental.

In the end, this is all about wanting three-arg converters again so you get to access attribute metadata.

I've done some more thinking in #146 (comment) – I feel like that would at least give you the tools to achieve what you want?

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

No branches or pull requests

4 participants