Add trait-names autocompletion support

[martinRenou]

cc @maartenbreddels @SylvainCorlay

Add a trait-names autocompletion support for HasTraits classes

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.

[maartenbreddels]

I think this is a 'dream coming true' for me. This increases usability so much!
Great work.

[rmorshea]

@martinRenou since inspect.Signature is new in 3.3 we'd have to add 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:

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 that you'd achieve the same effect - each time a new HasTraits class is defined this method will run. Its arguments (cls, classdict) are the newly defined class, and any new attributes it defined respectively.

[martinRenou]

So, I came up with this code (I will take your comment into account @maartenbreddels):

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__:

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:

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.

EDIT: Assigning to the __signature__ is only "safe" if the function actually takes kwargs

[martinRenou]

In order to support this use case:

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.

Also, when you do modify the signature, you should be sure to preserve the original as well by inserting 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.

index = None
parameters = list(sig.parameters.values())
for i, param in enumerate(parameters):
    if param.kind is Parameter.VAR_KEYWORD:
        index = i

if index is not None:
    parameters[index:index] = list_of_dynamic_trait_parameters

[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?

[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:

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 😄
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 don't think there's a way around replacing the __init__ method.

Given this, 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, ***@***.***> 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 <#515 (comment)>, 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.

from traitlets import HasTraits, has_autocompletion

@has_autocompletion
class MyClass(HasTraits):
    ....

VS

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 both).

Link to relevant Jedi issue: davidhalter/jedi#1058

You should open up an issue against IPython too if you don't think it will be fixed in Jedi.

[martinRenou]

@maartenbreddels @rmorshea The issue was fixed in Jedi and must be included in the last release (0.15)! 🎉

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

[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.

[martinRenou]

Replaced by #538

[rmorshea]

I think we should still have a PR against master. It would be really unfortunate if features in our 4.x.x branches start to diverge from master

[martinRenou]

Ok I will restore this PR then

[martinRenou]

I need to cherry-pick the commits from #538

[martinRenou]

Done :)

[maartenbreddels]

I think this will boost traitlets/widgets usability 1000 fold, many thanks @martinRenou for pushing on this, and thanks @rmorshea for reviewing this, this made my day :)
Really looking forward to seeing this released, and I'll make all my widget libraries use this :)