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

Type annotations #82

Closed
MSeal opened this issue Jun 16, 2020 · 16 comments
Closed

Type annotations #82

MSeal opened this issue Jun 16, 2020 · 16 comments
Assignees
Labels
enhancement New feature or request

Comments

@MSeal
Copy link
Contributor

MSeal commented Jun 16, 2020

We're on python 3 only, let's make use of type annotations on our functions.

@MSeal MSeal added the enhancement New feature or request label Jun 16, 2020
@davidbrochart davidbrochart self-assigned this Jun 17, 2020
@davidbrochart
Copy link
Member

Should we use stubs files or add type annotations in the source code?
Should we use python>=3.6 type annotation syntax?

@MSeal
Copy link
Contributor Author

MSeal commented Jun 17, 2020

Should we use stubs files or add type annotations in the source code?

I haven't used stubs much, just type annotations. I'm not sure how much benefit we'd get for using stubs? If you have more experience on that would love an opinion.

Should we use python>=3.6 type annotation syntax?

Yep

@choldgraf
Copy link
Contributor

Wait you mean we shouldn't interweave traitlets everywhere?

@choldgraf
Copy link
Contributor

(that is a joke, I think it's a good idea)

@davidbrochart
Copy link
Member

I haven't used stubs much, just type annotations. I'm not sure how much benefit we'd get for using stubs? If you have more experience on that would love an opinion.

I don't have any experience in type annotation, but I'm thinking that the Jupyter projects will need to exchange type definition, right? Like nbclient will probably need a definition of a KernelManager and a KernelClient I guess.

@MSeal
Copy link
Contributor Author

MSeal commented Jun 17, 2020

I don't have any experience in type annotation, but I'm thinking that the Jupyter projects will need to exchange type definition, right? Like nbclient will probably need a definition of a KernelManager and a KernelClient I guess.

Doesn't need a type stub -- you can just import the class and use it in the annotation inline. The inline annotations actually read better for expectations.

For example:

from jupyter_client.managerabc import KernelManagerABC
from nbformat import NotebookNode
...

def __init__(self, nb: NotebookNode, km: KernelManagerABC=None, **kw):
  ...

works just fine

@MSeal
Copy link
Contributor Author

MSeal commented Jun 17, 2020

Wait you mean we shouldn't interweave traitlets everywhere?

I actually don't know how traitlets play with type annotations... that would be something to figure out I guess (joke aside)

@choldgraf
Copy link
Contributor

my (poor) understanding of traitlets is that one of the reasons it was developed was to allow you to type inputs, so I think (?) to some degree they replicate functionality

@davidbrochart
Copy link
Member

Yes but traitlets validate type at runtime, it's not static type analysis.

@MSeal
Copy link
Contributor Author

MSeal commented Jun 17, 2020

They also provide a config interface for assignment from file / cml args.

@choldgraf
Copy link
Contributor

ok ok I apologize for throwing shade at traitlets :-)

@MSeal
Copy link
Contributor Author

MSeal commented Jun 17, 2020

lol, I don't particularly like the traitlet model either as it uses less than ideal software practices to achieve (high complexity in tracing, 10+ class inheritance chains, makes init operations more complex, very obscure error message patterns when it fails, unexpected config inheritance patterns, etc) -- which I've been vocal about. I'd prefer removing traitlets from nbclient if we could.

I've left them when porting over because the file based config is used very very heavily in nbconvert and the users who use it like it a lot. So removing it from nbclient would require mapping that in nbconvert in the preprocessor layer... might be worth discussing doing before 1.0 of nbclient and 6.0 of nbconvert fully ship? Would be tough to choose.

@davidbrochart
Copy link
Member

I guess if we just wanted the CLI part of traitlets, we could use click. But there's also the reactive part in traitlets (observing changes).

@choldgraf
Copy link
Contributor

(I'm a big fan of click)

@davidbrochart
Copy link
Member

I have started to work on it in #83.

@davidbrochart
Copy link
Member

Fixed in #83

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

No branches or pull requests

3 participants