manim icon indicating copy to clipboard operation
manim copied to clipboard

Add typings

Open PgBiel opened this issue 4 years ago • 22 comments

We should add typings to manim:

  1. It allows for more precise intellisense and easier-to-check code (via mypy and whatnot);
  2. It ensures we follow a structure that makes sense (i.e., ensures we don't have a data model in which people get completely lost);
  3. Writing documentation and specifying types are tied to each other. When we work on documentation, it is inevitable that we will specify types for things, so why not add it to the code as well?

Notes:

  1. We can use stubgen to have a headstart;
  2. We can use stub files in order to keep typings separate from the code and not have to change things much (but this is debatable - please expose your opinions on this);
  3. This is very related to the dataclasses solution proposed in #7, and may ultimately be a consequence of it.

Thoughts?

PgBiel avatar May 21 '20 15:05 PgBiel

One of the main problems with this is contributions from the community. What if a user wants to contribute a PR but they don't have experience with typing? Do we just reject all PRs without types, or do we guide each user in adding types? Or do we merge them and add the types ourselves?

leotrs avatar May 21 '20 15:05 leotrs

The latter is more appropriate. In general, the one making the PR should attempt to write the typings themselves. If they do not manage to, we can work this out by ourselves. (Not sure if the usage of stubs would prove itself more useful in this case)

PgBiel avatar May 21 '20 15:05 PgBiel

^we should at least expect PRs to have documentation, which makes writing typings by ourselves trivial

PgBiel avatar May 21 '20 15:05 PgBiel

^we should at least expect PRs to have documentation, which makes writing typings by ourselves trivial

Good point.

leotrs avatar May 21 '20 15:05 leotrs

It seems like we have a pretty solid plan for this. Should we get started?

yoshiask avatar May 21 '20 18:05 yoshiask

I think this should be left until after we are done with the "initial clean up" milestone.

leotrs avatar May 21 '20 19:05 leotrs

this should be done in conjunction with the dataclasses change for optimal compatibility and development.

PgBiel avatar May 21 '20 21:05 PgBiel

The latest developments in #7 kinda sound like we won't be using dataclasses. If that's the case, then we can start adding typings at any time, say one module at a time. Any takers?

leotrs avatar Oct 08 '20 21:10 leotrs

I think this is a good place to continue the discussion from https://github.com/ManimCommunity/manim/pull/483#discussion_r503470863

The points I was trying to make:

  • Type hints (PEP 484) are an excellent idea, I am all for adding them! A module-by-module approach sounds good to me, and I'd be happy to help implementing it.
  • Duplicating information (having typing information both in the form of type hints in method signatures as well as in docstrings) is a really bad idea; it will inevitably introduce inconsistencies.
  • Sphinx is able to deal with type hints and can format the html documentation so that it looks the same as rendering Numpy-style typing information in the docstring.
  • Type hints are picked up by modern IDEs (!)

I'd like to suggest changing our policy from adding typings via docstrings to adding typings via type hints. :-)

behackl avatar Oct 12 '20 20:10 behackl

Using in-code typings ~vs~ instead of in-docstring typings sounds like a good idea generally TBH.

leotrs avatar Oct 12 '20 20:10 leotrs

Using in-code typings vs in-docstring typings sounds like a good idea generally TBH.

Do you mean having typings in both of them, or separating them?

I'd like to suggest changing our policy from adding typings via docstrings to adding typings via type hints. :-)

As I said before: we may do that, however only after we actually get to work on in-code typehints. Until then, we need something to base ourselves on in order to generate typings.

PgBiel avatar Oct 12 '20 20:10 PgBiel

@PgBiel I meant having in-code and not having in-docstring.

What is the benefit of having both? All I could think of is that a user reading the docs might not get the full benefit from in-code, but if Sphinx can be configured to parse in-code and show those to the user, then why bother?

leotrs avatar Oct 12 '20 20:10 leotrs

Are we talking something like this: https://realpython.com/python39-new-features/#annotated-type-hints where you add both documentation about a function argument as well as its type information as an annotation in the function signature?

cobordism avatar Oct 12 '20 20:10 cobordism

@PgBiel I meant having in-code and not having in-docstring.

got it

What is the benefit of having both? All I could think of is that a user reading the docs might not get the full benefit from in-code, but if Sphinx can be configured to parse in-code and show those to the user, then why bother?

if we can configure that, then it should be fine The only thing that we have to note is that, at the moment, we aren't adding typehints in code, so they must be added in docstring for now Then a future typings PR may move them from the docstring to the code, and we may have simpler docstring types.

PgBiel avatar Oct 12 '20 20:10 PgBiel

Are we talking something like this: https://realpython.com/python39-new-features/#annotated-type-hints where you add both documentation about a function argument as well as its type information as an annotation in the function signature?

no no, this style of annotation was before the latest typing PEP took place, which ensured that annotations should be exclusively for typings (starting on py 3.5). We are talking about reproducing the typehint syntax in the documentation string of the class/function/... when specifying what a parameter or attribute is, for example.

PgBiel avatar Oct 12 '20 20:10 PgBiel

I have made #835 which type some parts of code and this can be done for the other parts of the code also.

naveen521kk avatar Dec 27 '20 16:12 naveen521kk

Seems like this never went anywhere? Bit of a shame. Makes it way harder to learn for beginners.

Timmmm avatar Sep 15 '21 18:09 Timmmm

Seems like this never went anywhere? Bit of a shame. Makes it way harder to learn for beginners.

It's work in progress. Type hints have been added to some parts of the library; help for adding them to other parts is warmly welcome.

behackl avatar Sep 15 '21 18:09 behackl

This is far too broad an issue to be resolved with one PR. For new contributors/hacktoberfest participants, I recommend working on one file per PR.

Darylgolden avatar Oct 01 '21 11:10 Darylgolden

I have added typings to most of Mobject.py. One tricky issue is Sequence[float] vs np.ndarray. They're not compatible types in general and it's not clear where you need to use np.ndarray and where you are allowed either.

I can put up a PR for discussion if you like?

Timmmm avatar Oct 01 '21 12:10 Timmmm

I have added typings to most of Mobject.py. One tricky issue is Sequence[float] vs np.ndarray. They're not compatible types in general and it's not clear where you need to use np.ndarray and where you are allowed either.

I can put up a PR for discussion if you like?

Sure, that sounds great!

Darylgolden avatar Oct 01 '21 12:10 Darylgolden

Here it is: #2129

I put TODO(types) comments where there are remaining type issues, as identified by Pyright (only at the first instance for each kind of issue).

Timmmm avatar Oct 01 '21 22:10 Timmmm

I generated typings stubs for manim locally with stubgen -p manim. I haven't tested this too much, but so far it's working well. Are there any official typing stubs for the project? Could we at least start with the generated stubs since they seem to work, and then improve things from there?

gsingh93 avatar Sep 26 '22 21:09 gsingh93