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 missing type annotations to _core/_mock_clock.py #2740

Merged
merged 4 commits into from
Aug 6, 2023

Conversation

CoolCat467
Copy link
Member

This PR adds missing type annotations to _core/_mock_clock.py.

@CoolCat467 CoolCat467 added the typing Adding static types to trio's interface label Aug 6, 2023
@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #2740 (0f62b4d) into master (49d0393) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2740   +/-   ##
=======================================
  Coverage   98.90%   98.90%           
=======================================
  Files         113      113           
  Lines       16683    16683           
  Branches     3025     3025           
=======================================
  Hits        16501    16501           
  Misses        125      125           
  Partials       57       57           
Files Changed Coverage Δ
trio/_core/_mock_clock.py 100.00% <100.00%> (ø)

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.

Just one small comment

@@ -62,7 +62,7 @@ class MockClock(Clock, metaclass=Final):

"""

def __init__(self, rate: float = 0.0, autojump_threshold: float = inf):
def __init__(self, rate: float = 0.0, autojump_threshold: float = inf) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Err, are we doing this or not...? Maybe put this in your bigger PR about these and punt decisions to that central place?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the changes I made with verify_types.json, it said that this PR adds 2 new typed functions, so I think this change will be important for 100% type completion for the entire project

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh does pyright require this? Could you check please? If so, I'm totally on board with adding -> None everywhere: that adds software to ensure we are consistently putting the -> None everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

This section of pep 484 talks about __init__ -> None annotations: https://peps.python.org/pep-0484/#the-meaning-of-annotations

Copy link
Member

Choose a reason for hiding this comment

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

With the changes I made with verify_types.json, it said that this PR adds 2 new typed functions, so I think this change will be important for 100% type completion for the entire project

That's because it counts the class itself as another symbol that's now fully typed. If you reverted only this one change it wouldn't change the output.

mypy has resolved that parenthetical in a different way, and seems like pyright does too here, in them not requiring -> None if all other parameters to __init__ are typed. I can't even get mypy to complain if I add --strict, so I'm a bit curious what configuration you're using that flags it.

@jakkdl jakkdl enabled auto-merge (squash) August 6, 2023 10:04
@jakkdl jakkdl merged commit 59229b1 into python-trio:master Aug 6, 2023
@CoolCat467 CoolCat467 deleted the typing-missing-mock-clock branch August 6, 2023 21:53
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.

4 participants