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

Add typings #52

Closed
PgBiel opened this issue May 21, 2020 · 23 comments · Fixed by #3086
Closed

Add typings #52

PgBiel opened this issue May 21, 2020 · 23 comments · Fixed by #3086
Labels
documentation Improvements or additions to documentation enhancement Additions and improvements in general good first issue Good for newcomers hacktoberfest

Comments

@PgBiel
Copy link
Member

PgBiel commented May 21, 2020

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 CONFIG dictionaries stuff  #7, and may ultimately be a consequence of it.

Thoughts?

@PgBiel PgBiel added documentation Improvements or additions to documentation enhancement Additions and improvements in general new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) labels May 21, 2020
@leotrs
Copy link
Contributor

leotrs commented May 21, 2020

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?

@PgBiel
Copy link
Member Author

PgBiel commented May 21, 2020

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
Copy link
Member Author

PgBiel commented May 21, 2020

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

@leotrs
Copy link
Contributor

leotrs commented May 21, 2020

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

Good point.

@yoshiask
Copy link
Contributor

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

@leotrs
Copy link
Contributor

leotrs commented May 21, 2020

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

@PgBiel
Copy link
Member Author

PgBiel commented May 21, 2020

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

@leotrs
Copy link
Contributor

leotrs commented Oct 8, 2020

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?

@behackl
Copy link
Member

behackl commented Oct 12, 2020

I think this is a good place to continue the discussion from #483 (comment)

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

@leotrs
Copy link
Contributor

leotrs commented Oct 12, 2020

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

@PgBiel
Copy link
Member Author

PgBiel commented Oct 12, 2020

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.

@leotrs
Copy link
Contributor

leotrs commented Oct 12, 2020

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

@cobordism
Copy link
Member

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?

@PgBiel
Copy link
Member Author

PgBiel commented Oct 12, 2020

@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
Copy link
Member Author

PgBiel commented Oct 12, 2020

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.

@naveen521kk
Copy link
Member

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

@Timmmm
Copy link
Contributor

Timmmm commented Sep 15, 2021

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

@behackl
Copy link
Member

behackl commented Sep 15, 2021

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.

@Darylgolden
Copy link
Member

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.

@behackl behackl pinned this issue Oct 1, 2021
@Timmmm
Copy link
Contributor

Timmmm commented Oct 1, 2021

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?

@Darylgolden
Copy link
Member

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!

@Timmmm
Copy link
Contributor

Timmmm commented Oct 1, 2021

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

@MrDiver MrDiver added this to To Do in Documentation via automation Jun 16, 2022
@MrDiver MrDiver removed the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label Jun 16, 2022
@MrDiver MrDiver moved this from To Do to Discussion in Documentation Jun 16, 2022
@gsingh93
Copy link

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?

@MrDiver MrDiver mentioned this issue Dec 25, 2022
@Viicos Viicos mentioned this issue Sep 22, 2023
22 tasks
Documentation automation moved this from Discussion to Done Nov 3, 2023
@behackl behackl unpinned this issue Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement Additions and improvements in general good first issue Good for newcomers hacktoberfest
Projects
Status: 🏗 In progress
Documentation
  
Done
Development

Successfully merging a pull request may close this issue.

10 participants