8000 Type Annotations by sakosha · Pull Request #188 · aio-libs/aioftp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Type Annotations #188

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open

Type Annotations #188

wants to merge 56 commits into from

Conversation

sakosha
Copy link
Contributor
@sakosha sakosha commented May 21, 2025 •
edited
Loading

What do these changes do?

Adding type annotations to user facing API, internals and bug-fixes discovered during type checking.

Are there changes in behavior for the user?

typing-extensions are now required dependency before py3.11.

users extending Client should replace self.stream w/ self._stream to preserve old behavior.

Related issue number

Fixes #164

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

These changes are large. I hope it won't be hard to review

@pohmelie
Copy link
Collaborator

pre-commit linters-set failed, run pre-commit run -a until all green and add changes to this pr. Also, I'm not sure this will work for python 3.9 in terms of modern type hinting.

@sakosha
Copy link
Contributor Author
sakosha commented May 22, 2025

Hello! I forgot to add typing-extensions as a dependency.

I don't quite get what you suggesting about pytohn3.9. Did y 8000 ou mean dropping support for it, or using only native features of it?

@codecov Codecov
Copy link
codecov bot commented May 22, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.63780% with 15 lines in your changes missing coverage. Please review.

Project coverage is 97.92%. Comparing base (e449dce) to head (7638b07).

Files with missing lines Patch % Lines
src/aioftp/pathio.py 95.41% 10 Missing ⚠️
src/aioftp/server.py 97.94% 3 Missing ⚠️
src/aioftp/client.py 99.31% 1 Missing ⚠️
src/aioftp/common.py 99.15% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   98.24%   97.92%   -0.32%     
==========================================
  Files           6        6              
  Lines        1933     2117     +184     
==========================================
+ Hits         1899     2073     +174     
- Misses         34       44      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pohmelie
Copy link
Collaborator

CI fails, fix please

I don't quite get what you suggesting about pytohn3.9

I was afraid of type hints won't work for python 3.9, but it looks like they are ok

@sakosha
Copy link
Contributor Author
sakosha commented May 22, 2025

could you try it again, please?

@pohmelie
Copy link
Collaborator

CI mypy fails @sakosha

@sakosha
Copy link
Contributor Author
sakosha commented May 25, 2025

Yep. I'll try troubleshooting it on Th.

@sakosha
Copy link
Contributor Author
sakosha commented Jun 1, 2025

Hello! type-check CI succeeded.

pohmelie
pohmelie requested changes Jun 1, 2025
View reviewed changes
@pohmelie
Copy link
Collaborator
pohmelie commented Jun 1, 2025

Good job @sakosha. I reviewed client and some other files. Did not look at server and pathio files, but lets start from client

@sakosha
Copy link
Contributor Author
sakosha commented Jun 11, 2025

hello! I believe TypedDict issue is the only one left.
I think we should left it as is, to not break our users' client code.

@pohmelie
Copy link
Collaborator

hello! I believe TypedDict issue is the only one left. I think we should left it as is, to not break our users' client code.

I need to review other files. Sorry for a delay, occupied at work hardly. Will try to review this on this week

pohmelie
pohmelie requested changes Jun 15, 2025
View reviewed changes
@pohmelie
Copy link
Collaborator

@sakosha

  • Please replace suffixes for types like P, R, T to fully named ParamSpec, TypeVar, Type for readability.
  • Pull changes from master
  • I will review server part after and we can finally merge this big work to master 🎉

@sakosha
Copy link
Contributor Author
sakosha commented Jun 15, 2025

Hi again! I believe I fixed all of the comments.

@pohmelie
Copy link
Collaborator

Hi again! I believe I fixed all of the comments.

Ok, will try to review everything tomorrow

@sakosha
Copy link
Contributor Author
sakosha commented Jun 15, 2025 •
edited
Loading

Also, I think we should ping others to see if they have any issues with the fork.
Of course after we done w/ code review.

@sakosha
Copy link
Contributor Author
sakosha commented Jun 16, 2025

Hello @pohmelie ! I forgot that dependencies can be optional on version.
I added

if sys.version_info >= (3, 11):
    from typing import ...
else:
    from typing_extensions import ...

to each major file to make typing-extensions optional

Sorry for the last minute changes.

@sakosha sakosha requested a review from pohmelie June 17, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohmelie pohmelie Awaiting requested review from pohmelie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add type hints to aioftp
2 participants
@sakosha @pohmelie
0