add type annotations (closes #164) #176
Conversation
…aramSpec compatible way
|
I have the first version of the pull request ready. I ran the tests on python 3.8 and python 3.12. There are some things that I think need to be clarified. Such as: |
| for fact in facts_found[:-1].split(";"): | ||
| key, _, value = fact.partition("=") | ||
| entry[key.lower()] = value | ||
| return pathlib.PurePosixPath(name), entry | ||
|
|
||
| return pathlib.PurePosixPath(name), cast(InfoDict, entry) |
There was a problem hiding this comment.
If you use cast between dictionaries in one place, the you should use it in all places. If you decide to make strict typing, then don't use cast.
There was a problem hiding this comment.
It is impossible to type every part of the code because python is very dynamic. It's also impossible to do without cast. The input is just bytes/strings and whether they are cast to the desired structure depends on who uses the function.
There was a problem hiding this comment.
In the other case here, you have to do some validation on a few lines and throw an error.
There was a problem hiding this comment.
In this case you can add a MLSxDict or something like that, which will use only required for further code keys (like "type") and type checker will help here, by the way. And after all InfoDict will be a union of three (or even more) type dicts.
There was a problem hiding this comment.
So, here I don't really understand what you're suggesting.
There was a problem hiding this comment.
I suggest to add MLSxDict with required fields and add it to InfoDict union.
| _PathType: TypeAlias = Union[str, pathlib.PurePosixPath] | ||
|
|
||
|
|
||
| _FAIL_CODE: TypeAlias = Literal["503", "425"] |
There was a problem hiding this comment.
Why this is a Literal? It looks like the ode can be any string
| message: str | ||
|
|
||
|
|
||
| _Future: TypeAlias = "asyncio.Future[Any]" |
There was a problem hiding this comment.
Why it is a "asyncio.Future" instead of asyncio.Future?
| user_required = ("user", "no user (use USER firstly)") | ||
| login_required = ("logged", "not logged in") | ||
| passive_server_started = ( | ||
| user_required: ClassVar[_ConnectionCondition] = _ConnectionCondition("user", "no user (use USER firstly)") |
There was a problem hiding this comment.
Since named tuples used, then there is no reasons to not to use names.
| timeout = getattr(cls, name) | ||
| return asyncio.wait_for(coro, timeout) | ||
| async def wrapper(*args: _PS.args, **kwargs: _PS.kwargs) -> _T: | ||
| self: "AsyncPathIO" = get_param((0, "self"), args, kwargs) |
There was a problem hiding this comment.
Why we need this get_param method, and not use types directly on function arguments definition?
| @ConnectionConditions( | ||
| ConnectionConditions.data_connection_made, | ||
| wait=True, | ||
| fail_code="425", | ||
| fail_info="Can't open data connection", | ||
| ) | ||
| @worker | ||
| async def stor_worker(self, connection, rest): | ||
| async def stor_worker(self: Self, connection: ConnectionProtocol, rest: object) -> None: |
There was a problem hiding this comment.
Why self is typed with Self only here if it needed?
| else: | ||
| code, info = "550", "path unreachable" | ||
| code, info = "550", ["path unreachable"] |
There was a problem hiding this comment.
Why add squared brackets only here? Other places are ok without brackets.
| @@ -1447,11 +1655,11 @@ async def _start_passive_server(self, connection, handler_callback): | |||
| ssl=self.ssl, | |||
| **self._start_server_extra_arguments, | |||
| ) | |||
| return passive_server | |||
| return passive_server # type: ignore | |||
There was a problem hiding this comment.
This looks like it is always Server
| from typing import Any, Dict, Tuple | ||
|
|
||
|
|
||
| def get_param(where: Tuple[int, str], args: Tuple[Any, ...], kwargs: Dict[str, Any]) -> Any: |
There was a problem hiding this comment.
This method (if it will be presented) should be in common I think
What do these changes do?
Pull request adds annotations to methods as well as some helper classes.
Are there changes in behavior for the user?
It is expected that nothing should change for users.
Related issue number
#164
Checklist