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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing type hints to __init__(...) #2938

Merged
merged 7 commits into from Nov 17, 2023

Conversation

pbelskiy
Copy link
Contributor

@pbelskiy pbelskiy commented Nov 12, 2023

Hello! Thanks for awesome package! 馃敟

First of all, I though about create an issue, but this is too small fix for that, dear maintainers
should I create an Issue or Discussion for that super small fixes?

Summary

Here I did little work for fixing of missed type hints of few of init()

According to https://peps.python.org/pep-0484/
(Note that the return type of __init__ ought to be annotated with -> None.

So type checkers (mypy, pyright) will work fine after this PR.

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.

@karpetrosyan
Copy link
Member

Hi @pbelskiy! Thanks for the pull request 馃檹

dear maintainers should I create an Issue or Discussion for that super small fixes?

We prefer discussion -> issue -> pull request path rather than code-change first, but I am okay reviewing this small change.

Yeah, this change makes sense, also because in some places we do not use annotation for __init__, though I found some classes where we use it.

Demo:

grep -irn __init__ tests

tests/test_multipart.py:385:        def __init__(self, iterator: typing.Iterator[bytes]) -> None:
tests/test_content.py:68:        def __init__(self, content: bytes) -> None:
tests/client/test_async_client.py:183:        def __init__(self) -> None:
tests/client/test_async_client.py:215:        def __init__(self, name: str):
tests/client/test_client.py:232:        def __init__(self) -> None:
tests/client/test_client.py:263:        def __init__(self, name: str):
tests/client/test_auth.py:27:    def __init__(self, auth_header: str = "", status_code: int = 200) -> None:
tests/client/test_auth.py:38:    def __init__(
tests/client/test_auth.py:96:    def __init__(self, repeat: int):
tests/client/test_auth.py:123:    def __init__(self, token: str) -> None:
tests/client/test_auth.py:142:    def __init__(self) -> None:

@karpetrosyan
Copy link
Member

Can you also add a changelog for this pr?

@pbelskiy
Copy link
Contributor Author

@karpetrosyan

Okay, I will add today later, thanks!

@pbelskiy
Copy link
Contributor Author

@karpetrosyan

From CHANGELOG.md
All notable changes to this project will be documented in this file.

Is adding few type hints are really notable?

@karpetrosyan
Copy link
Member

I think we should have a changelog, even if it's a little type change, so the user having type issues with the new release can understand what could be the reason.
We do not add a changelog if it's a tool change or the user cannot interact with that change.

Other changelog that we have:

Change the type of Extensions from Mapping[Str, Any] to MutableMapping[Str, Any]. (#2803)

Anyway, if we are sure that there are no regression chances, we can skip the changelog for this change.

@pbelskiy
Copy link
Contributor Author

Thanks, agree with you. So I've added info to changelog.

CHANGELOG.md Outdated Show resolved Hide resolved
@karpetrosyan karpetrosyan merged commit 87f39f1 into encode:master Nov 17, 2023
5 checks passed
@karpetrosyan karpetrosyan mentioned this pull request Nov 23, 2023
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