aio-libs / aioftp

ftp client/server for asyncio (http://aioftp.readthedocs.org)
Apache License 2.0
185 stars 54 forks source link

add type hints #165

Open honglei opened 1 year ago

honglei commented 1 year ago

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

pohmelie commented 1 year ago

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

honglei commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

mypy and pytest finally all passed.

honglei commented 1 year ago

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 commented 1 year ago

pathio.py:446

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

honglei commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

you are right! https://docs.python.org/3/library/ftplib.html#ftplib.FTP.dir

Nikita Melentev @.***> 于 2023年8月14日周一 下午11:49写道:

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.

— Reply to this email directly, view it on GitHub https://github.com/aio-libs/aioftp/pull/165#issuecomment-1677594544, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHW5AVF6JPIR3PI3S7ZIMLXVJCILANCNFSM6AAAAAAZ4GQOXA . You are receiving this because you authored the thread.Message ID: @.***>