Repository: ipython/traitlets
Pull request #515 (2019-03-29 10:24:36)

Add trait-names autocompletion support
cc @maartenbreddels @SylvainCorlay 

Add a trait-names autocompletion support for `HasTraits` classes
![completion_bqplot](https://user-images.githubusercontent.com/21197331/55225863-ecab0d00-5213-11e9-93bb-73270bd5918a.png)

Unfortunately, it does not work with `jedi`. Which means that this works for `ipython=7.1.1` by default, and it will work with other `ipython` versions **only if** `jedi` is not installed.

I failed to find a way to make it work with `jedi`.

rmorshea: @martinRenou since `inspect.Signature` is new in `3.3` we'd have to add [`funcsigs`](https://pypi.org/project/funcsigs/) as a dependency for Python<3.3
rmorshea: I also wonder whether you might be able to leverage `MetaHasTraits` so that this is enabled by default on all `HasTraits` classes instead of via a decorator.
martinRenou: The thing is I don't really know where to put this code if it's not in a decorator. Right now, because it's in a decorator, it gets executed when the class is created.
maartenbreddels: I used this snippet:
```python
def create_parameter(cls, name):
    trait = getattr(cls, name)
    if trait.default_value == traitlets.Undefined:
        default = Parameter.empty
    else:
        default = trait.default_value
    return Parameter(name=name, kind=Parameter.KEYWORD_ONLY, default=default)
```
rmorshea: @martinRenou I think if you were to drop this code right [here](https://github.com/ipython/traitlets/blob/master/traitlets/traitlets.py#L806) that you'd achieve the same effect.
martinRenou: So, I came up with this code (I will take your comment into account @maartenbreddels):

```python
class MetaHasTraits(MetaHasDescriptors):
    """A metaclass for HasTraits."""

    def setup_class(cls, classdict):
        cls._trait_default_generators = {}
        traits = [
            (name, value.default_value)
            for name, value in cls.class_traits().items()
            if not name.startswith('_')
        ]

        cls.__init__.__signature__ = Signature([
            Parameter(name, kind=Parameter.KEYWORD_ONLY, default=default)
            for name, default in traits
        ])

        super(MetaHasTraits, cls).setup_class(classdict)
```

But it is **REALLY** broken.
Say you have an `HasTraits` class which does not implement `__init__`:
```python
class Identity(HasTraits):
    username = Unicode()
```
Then in the `setup_class` method the `cls.__init__` is actually `HasTraits.__init__`, which means that I'm changing `HasTraits.__init__` signature...

Note that the problem still stands with the decorator implementation.
martinRenou: **Note to self:**

The following cannot be supported too:
```python
class Identity(HasTraits):
    username = Unicode()
    address = Unicode()

    def __init__(self, name): # The __init__ function does not take **kwargs
        super(Identity, self).__init__()
        self.username = name
```

The fact that the `__init__` function does not take `**kwargs` means that the it is not possible to pass `address`, so `address` should not be part of the signature.
martinRenou: In order to support this use case:
```python
class Identity(HasTraits):
    username = Unicode()
```
I create an `__init__` method on the fly in the MetaClass (Yeah I know it sounds bad...). Maybe there is a better way.
rmorshea: @martinRenou I don't think you should modify the `__init__` method on the fly. If someone removed `**kwargs` it might be for a reason. In this scenario should not modify the signature dynamically.

Also, when you do modify the signature, you should be sure to preserve the original as well. You should insert your KEYWORD_ONLY parameters here within the signature.

```python
def __init__(self, a, b, c, *args,   , **kwargs):
                                   ^
```

This will require you to read the original signature, find the index of a parameter whose `kind` is `VAR_KEYWORD` (if any) and then insert your dynamically defined trait parameters into the signature. If no `VAR_KEYWORD` parameters was found then you should not modify the signature.
martinRenou: Yeah I also thought about it for later improvements.

I'm only assigning a new `__init__` method if there is no. I'm not sure
what are the options in this case?

Le ven. 29 mars 2019 à 17:57, Ryan Morshead <notifications@github.com> a
écrit :

> @martinRenou <https://github.com/martinRenou> I don't think you should
> modify the __init__ method on the fly. If someone removed **kwargs it
> might be for a reason. In this scenario should not modify the signature
> dynamically.
>
> Also, when you do modify the signature, you should be sure to preserve the
> original as well. You should insert your KEYWORD_ONLY parameters here
> within the signature.
>
> def __init__(self, a, b, c, *args,   , **kwargs):
>                                    ^
>
> This will require you to read the original signature, find the index of a
> parameter whose kind is VAR_KEYWORD (if any) and then insert your
> dynamically defined trait parameters into the signature. If no VAR_KEYWORD
> parameters was found then you should not modify the signature.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ipython/traitlets/pull/515#issuecomment-478073042>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AUNyE3omSMVx1NHx9vKxL5hiUp4Y4Pprks5vbkYAgaJpZM4cSFE8>
> .
>

rmorshea: I can’t think of a real scenario where `__init__` wouldn’t be there, but even so, I wouldn’t dynamically assign an `__init__` method. That’s a bit too magical (even for traitlwts).
martinRenou: > I can’t think of a real scenario where __init__ wouldn’t be there

Well, this use case:
```python
class Identity(HasTraits):
    username = Unicode()
```

In this case `Identity.__init__` is actually `HasTraits.__init__` but I don't want to change `HasTraits.__init__` signature... That's why I create a new `__init__`. Do you see what I mean?

> I wouldn’t dynamically assign an __init__ method

I totally agree, this is bad. Please don't merge it I don't want to be responsible for that :smile:  
But I can't think of a better solution right now. 
rmorshea: @martinRenou of course! I see now - you'd be replacing the signature on the parent class.

I think your decorator idea is better.

As nice as it would be to have auto completion by default I think defining `__init__` on all `HasTraits` subclasses might be a problem:

1. Its quite "magical" - there's a real (though unlikely) possibility this causes a downstream bug somewhere.

2. There might be negative performance impacts since every instantiation would cause you to trace the `__init__` method of every class in the MRO.

You decorator solves these problems by:

1. Making the magic just a *little* more explicit.

2. Allowing you to remove the decorator if you're experiencing performance issues.
maartenbreddels: I think having an opt in method using decorators would be the safest
option. Thanks for helping out Ryan!

(from mobile phone)

On Sat, 30 Mar 2019, 07:04 Ryan Morshead, <notifications@github.com> wrote:

> @martinRenou <https://github.com/martinRenou> of course! I see now -
> you'd be replacing the signature on the parent class.
>
> I think your decorator idea is better.
>
> As nice as it would be to have auto completion by default I think defining
> __init__ on all HasTraits subclasses might be a problem:
>
>    1.
>
>    Its quite "magical" - there's a real (though unlikely) possibility
>    this causes a downstream bug somewhere.
>    2.
>
>    There might be negative performance impacts since every instantiation
>    would cause you to trace the __init__ method of every class in the MRO.
>
> You decorator solves these problems by:
>
>    1.
>
>    Making the magic just a *little* more explicit.
>    2.
>
>    Allowing you to remove the decorator if you're experiencing
>    performance issues.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ipython/traitlets/pull/515#issuecomment-478211455>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABryPY5igu90MvVvafE8LcSA0agexccRks5vbv5ggaJpZM4cSFE8>
> .
>

martinRenou: Or we could keep the implementation simple and unmagical in the `MetaHasTraits`. If the `__init__` method is actually the one of the base class, or if there is an `__init__` method but it does not take `kwargs`, we do nothing?

This way we could document that if one wants autocompletion in their traits class they simply need to implement `def __init__(self, **kwargs)`
rmorshea: @martinRenou I think that could be reasonable, however IMO the decorator looks nicer.

```python
from traitlets import has_autocompletion, HasTraits

@has_autocompletion
class MyClass(HasTraits):
    ....
```

VS

```python
from traitlets import HasTraits

class MyClass(HasTraits):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)
```
martinRenou: I agree it looks nicer. 

So if we go for the decorator, we need to make it really clear in the documentation that it creates an `__init__` function under the hood if there is no. And that it might not work if there is an `__init__` function that does not take `**kwargs`.

One thing that annoys me is that this does not work with `jedi`. `jedi` does not take into account the function `__signature__` for autocompletion, and IPython uses jedi by default. I guess we can try a fix in IPython so that it checks for the `__signature__` and uses `jedi` at the same time.
rmorshea: @martinRenou yes be sure to add documentation and tests (Traitlets needs more of those).

Link to relevant Jedi issue: https://github.com/davidhalter/jedi/issues/1058
martinRenou: @maartenbreddels @rmorshea The issue was fixed in Jedi and must be included in the last release (0.15)! :tada: 

I'll give this PR another shot and resolve your comments @rmorshea. And actually IPython depends on `jedi>=0.10`, so if this gets merged we should get `HasTraits` constructors completion for free in IPython and xeus-python.
martinRenou: So the is issue is indeed fixed. But we need to overwrite the `cls.__signature__`, not the `cls.__init__.__signature__`. I wonder if the second one is still needed.

Discussing with Maarten on gitter. We agree that the decorator solution is not only nicer, it's also less magical, and it won't mislead the user if the developer implements a custom `__init__` method that does not take `kwargs`.
martinRenou: What's remaining in this PR:

- [ ] Doc
- [ ] Tests
- [ ] Only expand the signature, and reuse the old signature for constructing the new one
rmorshea: I think I'm +1 on this except for the name of the decorator. I feel like `expand_constructor_signature` doesn't really describe what's happening which is adding trait names to the function signature. I'm open to alternatives, but maybe something like:

- `signature_has_traits` <- my vote is for this one.
- `add_traits_to_constructor`
- `constructor_with_traits`
rmorshea: Also, if we want to get this in a release you should make a PR with cherry picked commit against the `4.x` branch
martinRenou: Thanks for your comments @rmorshea, I will rename the decorator.

> Also, if we want to get this in a release you should make a PR with cherry picked commit against the 4.x branch

Shouldn't I open my PR against `4.3.x`? `4.x` was not updated since 3 years.
rmorshea: If I recall we haven't done a release off master in 3 years, but looking at it now I think minrk synced 4.x with 4.3.x recently
rmorshea: I think you could do it 4.3 though since this isn't a big feature add.

Patch:
diff --git a/docs/source/utils.rst b/docs/source/utils.rst
index 7867b94..df5bd1c 100644
--- a/docs/source/utils.rst
+++ b/docs/source/utils.rst
@@ -7,6 +7,30 @@ A simple utility to import something by its string name.
 
 .. autofunction:: import_item
 
+A way to expand the signature of the ``HasTraits`` class constructor. This enables auto-completion of trait-names in IPython and xeus-python when having Jedi>=0.15 by adding trait names with their default values in the constructor signature.
+Example:
+
+.. code:: Python
+
+    from inspect import signature
+
+    from traitlets import HasTraits, Int, Unicode, signature_has_traits
+
+    @signature_has_traits
+    class Foo(HasTraits):
+        number1 = Int()
+        number2 = Int()
+        value = Unicode('Hello')
+
+        def __init__(self, arg1, **kwargs):
+            self.arg1 = arg1
+
+            super(Foo, self).__init__(**kwargs)
+
+    print(signature(Foo))  # <Signature (arg1, *, number1=0, number2=0, value='Hello', **kwargs)>
+
+.. autofunction:: signature_has_traits
+
 Links
 -----
 
diff --git a/setup.py b/setup.py
index f12e85d..8ed8617 100644
--- a/setup.py
+++ b/setup.py
@@ -82,6 +82,7 @@ setuptools_args = {}
 install_requires = setuptools_args['install_requires'] = [
     'ipython_genutils',
     'six',
+    'funcsigs',
 ]
 
 extras_require = setuptools_args['extras_require'] = {
diff --git a/traitlets/__init__.py b/traitlets/__init__.py
index 09b93fe..532320f 100644
--- a/traitlets/__init__.py
+++ b/traitlets/__init__.py
@@ -3,6 +3,7 @@ from warnings import warn
 from . import traitlets
 from .traitlets import *
 from .utils.importstring import import_item
+from .utils.decorators import signature_has_traits
 from ._version import version_info, __version__
 
 
diff --git a/traitlets/utils/decorators.py b/traitlets/utils/decorators.py
new file mode 100644
index 0000000..807c2ac
--- /dev/null
+++ b/traitlets/utils/decorators.py
@@ -0,0 +1,83 @@
+"""Useful decorators for Traitlets users."""
+
+import copy
+
+try:
+    from inspect import Signature, Parameter, signature
+except ImportError:
+    from funcsigs import Signature, Parameter, signature
+
+from ..traitlets import Undefined
+
+
+def _get_default(value):
+    """Get default argument value, given the trait default value."""
+    return Parameter.empty if value == Undefined else value
+
+
+def signature_has_traits(cls):
+    """Return a decorated class with a constructor signature that contain Trait names as kwargs."""
+    traits = [
+        (name, _get_default(value.default_value))
+        for name, value in cls.class_traits().items()
+        if not name.startswith('_')
+    ]
+
+    # Taking the __init__ signature, as the cls signature is not initialized yet
+    old_signature = signature(cls.__init__)
+    old_parameter_names = list(old_signature.parameters)
+
+    old_positional_parameters = []
+    old_var_positional_parameter = None  # This won't be None if the old signature contains *args
+    old_keyword_only_parameters = []
+    old_var_keyword_parameter = None  # This won't be None if the old signature contains **kwargs
+
+    for parameter_name in old_signature.parameters:
+        # Copy the parameter
+        parameter = copy.copy(old_signature.parameters[parameter_name])
+
+        if parameter.kind is Parameter.POSITIONAL_ONLY or parameter.kind is Parameter.POSITIONAL_OR_KEYWORD:
+            old_positional_parameters.append(parameter)
+
+        elif parameter.kind is Parameter.VAR_POSITIONAL:
+            old_var_positional_parameter = parameter
+
+        elif parameter.kind is Parameter.KEYWORD_ONLY:
+            old_keyword_only_parameters.append(parameter)
+
+        elif parameter.kind is Parameter.VAR_KEYWORD:
+            old_var_keyword_parameter = parameter
+
+    # Unfortunately, if the old signature does not contain **kwargs, we can't do anything,
+    # because it can't accept traits as keyword arguments
+    if old_var_keyword_parameter is None:
+        raise RuntimeError(
+            'The {} constructor does not take **kwargs, which means that the signature can not be expanded with trait names'
+            .format(cls)
+        )
+
+    new_parameters = []
+
+    # Append the old positional parameters (except `self` which is the first parameter)
+    new_parameters += old_positional_parameters[1:]
+
+    # Append *args if the old signature had it
+    if old_var_positional_parameter is not None:
+        new_parameters.append(old_var_positional_parameter)
+
+    # Append the old keyword only parameters
+    new_parameters += old_keyword_only_parameters
+
+    # Append trait names as keyword only parameters in the signature
+    new_parameters += [
+        Parameter(name, kind=Parameter.KEYWORD_ONLY, default=default)
+        for name, default in traits
+        if name not in old_parameter_names
+    ]
+
+    # Append **kwargs
+    new_parameters.append(old_var_keyword_parameter)
+
+    cls.__signature__ = Signature(new_parameters)
+
+    return cls


Test patch:
diff --git a/traitlets/utils/tests/test_decorators.py b/traitlets/utils/tests/test_decorators.py
new file mode 100644
index 0000000..fc38731
--- /dev/null
+++ b/traitlets/utils/tests/test_decorators.py
@@ -0,0 +1,141 @@
+from unittest import TestCase
+try:
+    from inspect import Signature, Parameter, signature
+except ImportError:
+    from funcsigs import Signature, Parameter, signature
+
+from ...traitlets import HasTraits, Int, Unicode
+
+from ..decorators import signature_has_traits
+
+
+class TestExpandSignature(TestCase):
+
+    def test_no_init(self):
+        @signature_has_traits
+        class Foo(HasTraits):
+            number1 = Int()
+            number2 = Int()
+            value = Unicode('Hello')
+
+        parameters = signature(Foo).parameters
+        parameter_names = list(parameters)
+
+        self.assertIs(parameters['args'].kind, Parameter.VAR_POSITIONAL)
+        self.assertEqual('args', parameter_names[0])
+
+        self.assertIs(parameters['number1'].kind, Parameter.KEYWORD_ONLY)
+        self.assertIs(parameters['number2'].kind, Parameter.KEYWORD_ONLY)
+        self.assertIs(parameters['value'].kind, Parameter.KEYWORD_ONLY)
+
+        self.assertIs(parameters['kwargs'].kind, Parameter.VAR_KEYWORD)
+        self.assertEqual('kwargs', parameter_names[-1])
+
+        f = Foo(number1=32, value='World')
+        self.assertEqual(f.number1, 32)
+        self.assertEqual(f.number2, 0)
+        self.assertEqual(f.value, 'World')
+
+    def test_partial_init(self):
+        @signature_has_traits
+        class Foo(HasTraits):
+            number1 = Int()
+            number2 = Int()
+            value = Unicode('Hello')
+
+            def __init__(self, arg1, **kwargs):
+                self.arg1 = arg1
+
+                super(Foo, self).__init__(**kwargs)
+
+        parameters = signature(Foo).parameters
+        parameter_names = list(parameters)
+
+        self.assertIs(parameters['arg1'].kind, Parameter.POSITIONAL_OR_KEYWORD)
+        self.assertEqual('arg1', parameter_names[0])
+
+        self.assertIs(parameters['number1'].kind, Parameter.KEYWORD_ONLY)
+        self.assertIs(parameters['number2'].kind, Parameter.KEYWORD_ONLY)
+        self.assertIs(parameters['value'].kind, Parameter.KEYWORD_ONLY)
+
+        self.assertIs(parameters['kwargs'].kind, Parameter.VAR_KEYWORD)
+        self.assertEqual('kwargs', parameter_names[-1])
+
+        f = Foo(1, number1=32, value='World')
+        self.assertEqual(f.arg1, 1)
+        self.assertEqual(f.number1, 32)
+        self.assertEqual(f.number2, 0)
+        self.assertEqual(f.value, 'World')
+
+    def test_duplicate_init(self):
+        @signature_has_traits
+        class Foo(HasTraits):
+            number1 = Int()
+            number2 = Int()
+
+            def __init__(self, number1, **kwargs):
+                self.test = number1
+
+                super(Foo, self).__init__(number1=number1, **kwargs)
+
+        parameters = signature(Foo).parameters
+        parameter_names = list(parameters)
+
+        self.assertListEqual(parameter_names, ['number1', 'number2', 'kwargs'])
+
+        f = Foo(number1=32, number2=36)
+        self.assertEqual(f.test, 32)
+        self.assertEqual(f.number1, 32)
+        self.assertEqual(f.number2, 36)
+
+    def test_full_init(self):
+        @signature_has_traits
+        class Foo(HasTraits):
+            number1 = Int()
+            number2 = Int()
+            value = Unicode('Hello')
+
+            def __init__(self, arg1, arg2=None, *pos_args, **kw_args):
+                self.arg1 = arg1
+                self.arg2 = arg2
+                self.pos_args = pos_args
+                self.kw_args = kw_args
+
+                super(Foo, self).__init__(*pos_args, **kw_args)
+
+        parameters = signature(Foo).parameters
+        parameter_names = list(parameters)
+
+        self.assertIs(parameters['arg1'].kind, Parameter.POSITIONAL_OR_KEYWORD)
+        self.assertEqual('arg1', parameter_names[0])
+
+        self.assertIs(parameters['arg2'].kind, Parameter.POSITIONAL_OR_KEYWORD)
+        self.assertEqual('arg2', parameter_names[1])
+
+        self.assertIs(parameters['pos_args'].kind, Parameter.VAR_POSITIONAL)
+        self.assertEqual('pos_args', parameter_names[2])
+
+        self.assertIs(parameters['number1'].kind, Parameter.KEYWORD_ONLY)
+        self.assertIs(parameters['number2'].kind, Parameter.KEYWORD_ONLY)
+        self.assertIs(parameters['value'].kind, Parameter.KEYWORD_ONLY)
+
+        self.assertIs(parameters['kw_args'].kind, Parameter.VAR_KEYWORD)
+        self.assertEqual('kw_args', parameter_names[-1])
+
+        f = Foo(1, 3, 45, 'hey', number1=32, value='World')
+        self.assertEqual(f.arg1, 1)
+        self.assertEqual(f.arg2, 3)
+        self.assertTupleEqual(f.pos_args, (45, 'hey'))
+        self.assertEqual(f.number1, 32)
+        self.assertEqual(f.number2, 0)
+        self.assertEqual(f.value, 'World')
+
+    def test_no_kwargs(self):
+        with self.assertRaises(RuntimeError):
+            @signature_has_traits
+            class Foo(HasTraits):
+                number1 = Int()
+                number2 = Int()
+
+                def __init__(self, arg1, arg2=None):
+                    pass