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

mypy annotation for test_dos.py #19036

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

mypy annotation for test_dos.py #19036

wants to merge 2 commits into from

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Dec 12, 2024

Purpose:

Remove unused code and introduce stricter type checking of test_dos.py, gradually getting closer to full mypy coverage.

Current Behavior:

test_dos.py does not have full mypy coverage.

There are 3 copies of an unused function, get_block_path().

New Behavior:

test_dos.py has full mypy coverage (the the extent our repo is configured).

Unused function get_block_path() has been removed.

Testing Notes:

@arvidn arvidn added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Tests Changes to tests labels Dec 12, 2024
@arvidn arvidn changed the title Test cleanup mypy annotation for test_dos.py Dec 12, 2024
@arvidn arvidn marked this pull request as ready for review December 12, 2024 11:52
@arvidn arvidn requested a review from a team as a code owner December 12, 2024 11:52
@arvidn arvidn requested a review from AmineKhaldi December 12, 2024 11:52
@@ -307,13 +315,13 @@ def is_closed():
assert not ws_con.closed

# Remove outbound rate limiter to test inbound limits
ws_con.outbound_rate_limiter = FakeRateLimiter()
ws_con.outbound_rate_limiter = FakeRateLimiter() # type: ignore[assignment]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do without this type ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not without coming up with a different solution to make things unlimited

Copy link
Contributor

Choose a reason for hiding this comment

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

Sent #19038 to address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it's an improvement. it just hides the type-unsafety

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Tests Changes to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants