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 type hints in test_base.py #2445

Merged
merged 13 commits into from
Feb 3, 2024
Merged

Conversation

TechNiick
Copy link
Contributor

@TechNiick TechNiick commented Jan 26, 2024

Summary

This is part of a bigger Task. The idea is to remove those flags on mypy check command on: scripts/check.

Before
${PREFIX}mypy starlette
${PREFIX}mypy tests --disable-error-code no-untyped-def --disable-error-code no-untyped-call

After
${PREFIX}mypy starlette tests

In order to do that, we need to fix all issues after deleting those flags. After deleting those, there are more than 730 issues, in 34 files. I will make separate PRs for each file in order to have atomic changes.

Things done in this PR:

  • Added type annotations to function parameters and return type that were requested by mypy in test_base.py file.
  • Added used types in the types.py files.
  • Changed types that were already in types.py in places where the types were used directly

NOTES:

  • There are some places where i have added "Any" type since i couldn't figure out what types should be there. If anyone has any idea what types should be there, please let me know.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@TechNiick TechNiick changed the title Type annotations to test-base.py Type annotations to test_base.py Jan 26, 2024
@TechNiick TechNiick changed the title Type annotations to test_base.py Type annotations in test_base.py Jan 26, 2024
Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Besides this, looks good. Thanks! 🙏

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The types only used on the test suite shouldn't be added here. You can add them in the test_base.py itself for now, and if reused on multiple files, we can have them added in a module on the tests/ directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review and the guidance!

I've noticed that certain types, such as TestClientFactory, are indeed reused across multiple test files (e.g., test_routing.py, test_applications.py, test_background.py, etc.). To confirm my understanding and plan of action:

  1. Create a New Module for Shared Test Types: I will create a new module within the tests/ directory to house these shared types. I'm considering naming it tests.test_types.types. Does this naming convention work for you, or would you suggest a different structure?

  2. Revert Changes to starlette.types: I understand that the starlette.types module is intended for application-level types. Therefore, I'll move the test-specific types that I have added out of this module and into the newly created module for shared test types.

  3. Handling Types Used in a Single File: For types that are currently only used in one test file, I plan to keep them within their respective files to maintain simplicity. However, now that we'll have a dedicated module for shared test types (test_types.types), would you prefer moving all test-specific types there for consistency, or should they remain in their individual test files until needed elsewhere?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

No. For now, just the test_base.py. Create the types you need there. We'll talk if they are needed or not individually, and if we should move them somewhere.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Let's go step by step. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thank you!

I have done the requested changes, you can check.

@TechNiick TechNiick requested a review from Kludex February 3, 2024 14:02
Comment on lines 28 to 31
BytesGenerator = Generator[bytes, None, None]
AwaitableGenerator = Generator[Any, None, None]
TestClientFactory = Callable[[ASGIApp], TestClient]
MiddlewareClass = Type[_MiddlewareClass[Any]]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can you instead not create those aliases, please? You can leave the TestClientFactory here, but the ones that are used only once, I'd prefer to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, deleted them.

@TechNiick TechNiick requested a review from Kludex February 3, 2024 15:59
@Kludex
Copy link
Sponsor Member

Kludex commented Feb 3, 2024

Thanks! :)

@TechNiick TechNiick changed the title Type annotations in test_base.py Type hints in test_base.py Feb 4, 2024
@TechNiick TechNiick changed the title Type hints in test_base.py Add type hints in test_base.py Feb 6, 2024
nixroxursox pushed a commit to nixroxursox/starlette that referenced this pull request Mar 18, 2024
* added type annotations to test-base.py

* deleted unused imports

* fixed import order

* conditional import

* conditional import TestClient on types.py

* using string literals when importing in types

* added missing imports

* deleted starlette/types, refactored test_base types

* deleted types

---------

Co-authored-by: Scirlat Danut <scirlatdanut@scirlats-mini.lan>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants