traitlets icon indicating copy to clipboard operation
traitlets copied to clipboard

Collating list traits fails when only one is given - regression in traitlets 5.13.0

Open krassowski opened this issue 1 year ago • 2 comments

See https://github.com/jupyterlab/jupyter-ai/issues/913. Introduced in https://github.com/ipython/traitlets/pull/885 which should have no side effects as it was meant to only update types.

  • 952d6a7e6ad4fb971a8d209b1aea108731641dec (Publish 5.12.0) good
  • 3a03835f9d2d837ee95da1b5fdb3f505528b8585 (https://github.com/ipython/traitlets/pull/884) good
  • 4f0e68b7f9c4f42227c24ab15ff126e792f99098 (https://github.com/ipython/traitlets/pull/885) bad

krassowski avatar Jul 26 '24 19:07 krassowski

The regression is caused by a change in traitlets/traitlets.py file, in Instance class, in validate method:

-    def validate(self, obj, value):
+    def validate(self, obj: t.Any, value: t.Any) -> T | None:
         assert self.klass is not None
+        if self.allow_none and value is None:
+            return value
         if isinstance(value, self.klass):  # type:ignore[arg-type]
-            return value
+            return t.cast(T, value)
         else:
             self.error(obj, value)

Nothing obviously wrong here, but apparently the Container logic depends on the error being thrown here :shrug:

krassowski avatar Jul 26 '24 19:07 krassowski

The self.error() call was previously making the logic work correctly because the error is excepted to be raised by DeferredConfigString.get_value():

https://github.com/ipython/traitlets/blob/13de53c537a257402035139dd6862558eb19d362/traitlets/config/loader.py#L390-L417

get_value() calls trait.from_string():

https://github.com/ipython/traitlets/blob/13de53c537a257402035139dd6862558eb19d362/traitlets/traitlets.py#L3511-L3519

which calls validate().

So if we pass --ServerApp.a_list=a_value where a_list = List(allow_none=True) previously we would get a_list = ['a_value'] and now we are getting a_list = None.

krassowski avatar Jul 26 '24 19:07 krassowski