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 type hints #165

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

add type hints #165

wants to merge 33 commits into from

Conversation

honglei
Copy link

@honglei honglei commented Jul 3, 2023

tested under Debian10/python3.7, Debian10/python3.11,

.github/workflows/ci.yml Outdated Show resolved Hide resolved
aioftp/client.py Outdated Show resolved Hide resolved
aioftp/client.py Outdated Show resolved Hide resolved
aioftp/client.py Outdated Show resolved Hide resolved
aioftp/client.py Outdated Show resolved Hide resolved
aioftp/client.py Outdated Show resolved Hide resolved
@pohmelie
Copy link
Collaborator

pohmelie commented Jul 4, 2023

If we implement types, then we need mypy on CI of course.

aioftp/client.py Outdated Show resolved Hide resolved
Copy link
Author

@honglei honglei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto-formated by black,
add missing type hints,
add ftp command SIZE in tests.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
aioftp/client.py Outdated Show resolved Hide resolved
aioftp/client.py Outdated Show resolved Hide resolved
@honglei
Copy link
Author

honglei commented Jul 11, 2023

I prefer pyftpdlib-style naming for FTP command process hander:
type -> ftp_TYPE
list -> ftp_LIST

  1. type and list are python key-words
  2. these functions are used internal in class Server, so the change may not affect lib users.
  3. the prefix ftp_ is more clear than current names.

@pohmelie
Copy link
Collaborator

I prefer pyftpdlib-style naming for FTP command process hander: type -> ftp_TYPE list -> ftp_LIST

1. `type` and `list` are python key-words

2. these functions are used internal in class `Server`, so the change may not affect lib users.

3. the prefix `ftp_` is more clear than current names.

It's okay to have a list method, since method does not shadow original list.

image

@honglei
Copy link
Author

honglei commented Jul 18, 2023

If we implement types, then we need mypy on CI of course.

There is no easy way to achieve this. like the following problems:
https://discuss.python.org/t/improve-typing-with-to-force-not-none-value/7840/3

@pohmelie
Copy link
Collaborator

pohmelie commented Aug 5, 2023

There is no easy way to achieve this. like the following problems:
https://discuss.python.org/t/improve-typing-with-to-force-not-none-value/7840/3
Just add check before using result:

t: T | None = f()
if t is None:
    ...
t.bar()

@honglei
Copy link
Author

honglei commented Aug 5, 2023

There is no easy way to achieve this. like the following problems:
https://discuss.python.org/t/improve-typing-with-to-force-not-none-value/7840/3
Just add check before using result:

t: T | None = f()
if t is None:
    ...
t.bar()

@pohmelie thanks,just learned from https://mypy.readthedocs.io/en/stable/type_narrowing.html

@honglei
Copy link
Author

honglei commented Aug 8, 2023

mypy and pytest finally all passed.

@honglei honglei requested a review from pohmelie August 8, 2023 13:33
Copy link
Collaborator

@pohmelie pohmelie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix all mypy errors. I expand linters with ruff (instead of flake8) and black formatter. And targeted python version 3.11.

@honglei
Copy link
Author

honglei commented Aug 13, 2023

use python keyworkd list as function name in pyathio.py/PathIO caused some problems:

async def _open(self, path: pathlib.Path, *args: List[Any], **kwargs: dict[Any, Any]) -> BufferedRandom:

ruff:

aioftp/pathio.py:446:54: UP006 Use `list` instead of `List` for type annotation

Changed to list:

async def _open(self, path: pathlib.Path, *args: list[Any], **kwargs: dict[Any, Any]) -> BufferedRandom:

mypy:

pathio.py:446:54: error: Function "aioftp.pathio.PathIO.list" is not valid as a
type  [valid-type]
    ...async def _open(self, path: pathlib.Path, *args: list[Any], **kwargs: ...
                                                        ^
pathio.py:446:54: note: Perhaps you need "Callable[...]" or a callback protocol?
pathio.py:447:16: error: No overload variant of "open" of "Path" matches
argument types "tuple[list?[Any], ...]", "dict[str, dict[Any, Any]]" 
[call-overload]

@pohmelie
Copy link
Collaborator

pathio.py:446

Just add type alias for this, something like ListType = list at top of the file.

@honglei
Copy link
Author

honglei commented Aug 14, 2023

ListType = list

Good idea, but bad ideas of reusing the name of build-in function name list. https://stackoverflow.com/questions/9109333/is-it-bad-practice-to-use-a-built-in-function-name-as-an-attribute-or-method-ide

@pohmelie
Copy link
Collaborator

ListType = list

Good idea, but bad ideas of reusing the name of build-in function name list. https://stackoverflow.com/questions/9109333/is-it-bad-practice-to-use-a-built-in-function-name-as-an-attribute-or-method-ide

It won't be changed. pathio.list() will remain pathio.list() it doesn't shadow original list. So lets make a workaround here, since it just type checks, not a real features/improvements or something. Think about it as what API I want to use, I think it is obvious, that pathio.list() is what client expect to see.

@honglei
Copy link
Author

honglei commented Aug 14, 2023 via email

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

Successfully merging this pull request may close these issues.

2 participants