Skip to content

Test the Version class #287

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

Merged
merged 2 commits into from
Apr 15, 2025
Merged

Test the Version class #287

merged 2 commits into from
Apr 15, 2025

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Apr 13, 2025

No description provided.

from build_docs import Version


def test_equality() -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Are all the -> None necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, type annotations are completely optional in Python :)

More seriously, they could be omitted, as we're not running mypy on this repo (yet).

And generally, it is a good idea to also type hints to tests (for example, see https://sethmlarson.dev/tests-arent-enough-case-study-after-adding-types-to-urllib3#type-your-tests) and that could cover these returns too.

It can be useful in tests to help immediately differentiate from fixtures and helper methods which do return something, for example in #288.

Copy link
Member

Choose a reason for hiding this comment

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

I was referring specifically to the tests (not fixtures or other helper functions), since they (always?) return None, making the -> None superfluous.
I guess it makes sense to add it for consistency, both with other functions/methods that have the return type, or for tests that have types for the their args?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can remove them if you prefer, I don't mind too much either way. But when we add mypy, let's add them back if it complains about it.

Copy link
Member

Choose a reason for hiding this comment

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

Adding them opt-in the function to be checked by mypy, so it will be usefull if we use mypy on this repo sometime.

@AA-Turner AA-Turner merged commit 9f45c4e into python:main Apr 15, 2025
10 checks passed
@hugovk hugovk deleted the test-version branch April 16, 2025 07:42
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.

4 participants