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

Fix some type issues with trio.Path #2815

Merged
merged 15 commits into from
Oct 17, 2023

Conversation

TeamSpen210
Copy link
Contributor

After testing my own code against the new hints, I noticed that I messed up the path stubs a little. This fixes them in a more principled way, by going through the definitions in the same order that the typeshed stubs do for pathlib.Path. Things inherited from PurePath should be sync, while Path methods are async (some were mixed up). To ensure all the definitions are correct, I wrote a type-test which just uses every method to check it's doing the right thing.

@TeamSpen210 TeamSpen210 added the typing Adding static types to trio's interface label Oct 9, 2023
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #2815 (0610fc2) into master (3d62ea0) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2815   +/-   ##
=======================================
  Coverage   99.13%   99.13%           
=======================================
  Files         115      115           
  Lines       17230    17242   +12     
  Branches     3084     3085    +1     
=======================================
+ Hits        17081    17093   +12     
  Misses        104      104           
  Partials       45       45           
Files Coverage Δ
trio/_file_io.py 100.00% <ø> (ø)
trio/_path.py 100.00% <100.00%> (ø)
trio/_tests/test_exports.py 97.05% <100.00%> (+0.03%) ⬆️
trio/_tests/test_file_io.py 100.00% <100.00%> (ø)
trio/_tests/test_path.py 100.00% <100.00%> (ø)

@TeamSpen210
Copy link
Contributor Author

Also fixing an interesting docs build error caused by Python 3.12 releasing. pathlib.Path.link_to() was deprecated and removed, causing our link in the docs to fail. Added a manual deprecation to the docs.

trio/_path.py Show resolved Hide resolved
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

I don't see anything glaringly obvious but this is a big diff for what feels like pretty minor changes so I also glossed over a ton...

trio/_tests/test_exports.py Show resolved Hide resolved
"suffix",
"suffixes",
}
assert len(extra) == before - 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Codecov warnings here and above seem out of place. Is that just some codecov flakiness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems fine on the Codecov site, so it's probably just GitHub integration not working perfectly.

@@ -0,0 +1,118 @@
"""Path wrapping is quite complex, ensure all methods are understood as wrapped correctly."""
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish there was a nicer way to check we conform to typeshed's type hints and I think there might be other places where that would benefit us. But I'm not sure really how and it's probably worth punting that to another PR if we ever figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's tricky, since we're not actually entirely conforming - we switch to async of course, and automatically replace pathlib.Pathtrio.Path. Perhaps the best way might be to do things like code generation to create the tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just defer on this. Let's merge this PR!

@jakkdl
Copy link
Member

jakkdl commented Oct 15, 2023

I think you should take this opportunity to enable type-checking on trio/_tests/test_path and fix any problems in it - it might have caught these errors if it was enabled originally.

@TeamSpen210 TeamSpen210 merged commit 4b2fd98 into python-trio:master Oct 17, 2023
29 of 30 checks passed
@TeamSpen210 TeamSpen210 deleted the path-fixes branch November 8, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Adding static types to trio's interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants