google / mediapy

This Python library makes it easy to display images and videos in a notebook.
https://pypi.org/project/mediapy/
Apache License 2.0
389 stars 18 forks source link

Use modern type annotations #23

Closed hhoppe closed 1 year ago

hhoppe commented 1 year ago

(No functional changes.)

Use modern annotations (using from __future__ import annotations) and types in collections.abc.

In pyproject.toml, added configurations for autopep8, mypy, and pylint so that these tools report no errors.

echo autopep8; autopep8 -j8 -d .; echo mypy; mypy .; echo pylint; pylint -j8 .; echo pytest; pytest -qq

I verified that all works fine in Colab using !pip install git+https://github.com/hhoppe/mediapy.git@annotations in several notebooks.

My next step is to update the online API documentation, to use pdoc rather than pdoc3, and to use an automated GitHub workflow action.

Conchylicultor commented 1 year ago

Thank you, unfortunately Google is still running under Python 3.9. So pytype trigger internally:

unsupported operand type(s) for |: '_Path: Type[Union[str, os.PathLike]]' and 'None: None'

We're planning to switch to 3.10 around January 15th. After which I think this could be submitted.

However be aware this would trigger error if users try to explicitly access the typing annotations in 3.8 and 3.9:

from __future__ import annotations

def f() -> int | str:  # Works in 3.8
  pass

typing_extensions.get_type_hints(f)  # TypeError in 3.8

Raise in 3.9 and 3.8

TypeError: unsupported operand type(s) for |: 'type' and 'type'

But it's unlikely that user try typing_extensions.get_type_hints on mediapy API, so we can still submit this after Google use 3.10 by default

hhoppe commented 1 year ago

Thanks! January 15 is not far at all --- keeping fingers crossed.. I spent some time today looking at "pyink"; it seems very nice.

On Mon, Jan 9, 2023 at 4:29 AM Conchylicultor @.***> wrote:

@.**** commented on this pull request.

In pyproject.toml https://github.com/google/mediapy/pull/23#discussion_r1064587265:

@@ -45,3 +45,35 @@ dev = [ [build-system] requires = ["flit_core >=3.5,<4"] build-backend = "flit_core.buildapi" + +[tool.autopep8]

(Maybe not in this cl) Can we use pyink instead ? This is becoming the Google default, a fork of black adapted to Google style. See: https://github.com/google/pyink

[tool.pyink]# Formatting configuration to follow Google style-guidepyink-indentation = 2pyink-use-majority-quotes = true

And for vscode compatibility: https://github.com/google/pyink#is-there-a-vs-code-extension-for-pyink

In pyproject.toml https://github.com/google/mediapy/pull/23#discussion_r1064588054:

+[tool.autopep8] +indent_size = 2 +max_line_length = 80 +ignore = "E121,E125,E126,E129,E226,E302,E305,E501,W504,E741" +aggressive = 3 +recursive = true + +[tool.mypy] +strict = true +ignore_missing_imports = true + +[[tool.mypy.overrides]] +module = "mediapy_test" +ignore_errors = true + +[tool.pylint.main]

(Just for info) Alternatively there's a google-specific .pylintrc: https://github.com/google-research/visu3d/blob/main/.pylintrc

— Reply to this email directly, view it on GitHub https://github.com/google/mediapy/pull/23#pullrequestreview-1240406197, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACYGNYQKTBLWQBR7O6FUMUDWRQABBANCNFSM6AAAAAATUOFTME . You are receiving this because you authored the thread.Message ID: @.***>

Conchylicultor commented 1 year ago

For pyink, see https://github.com/google-research/visu3d/pull/89/files as how I setup it in my other projects

Conchylicultor commented 1 year ago

Our tests now pass at Google. I'll still wait a few days before submitting this in case 3.10 is rolled back internally

Conchylicultor commented 1 year ago

Submitting this now. Thank you for your patience!

Conchylicultor commented 1 year ago

Closing this as this was merged in https://github.com/google/mediapy/commit/5270f0e8d180721cb52e8c43955cb1754ad11bdf. Not sure why copybara hasn't closed this