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

Update __init__ return typing to be consistent #506

Merged
merged 5 commits into from
Sep 22, 2022

Conversation

Harshal6927
Copy link
Contributor

@Harshal6927 Harshal6927 commented Sep 22, 2022

PR Checklist

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you got 100% test coverage on new code?
  • Have you updated the prose documentation?
  • Have you updated the reference documentation?

@Goldziher
Copy link
Contributor

Ok, if we want to go this path we should actually use the NoReturn type in all init methods.

@peterschutt
Copy link
Contributor

Ok, if we want to go this path we should actually use the NoReturn type in all init methods.

Mypy doesn't let you do that:

src/worker/parse.py:14: error: The return type of "__init__" must be None  [misc]
Found 1 error in 1 file (checked 92 source files)

Seems NoReturn is for functions that can never return, e.g., always raise:

Special type indicating that a function never returns.

Some refs:

@Goldziher
Copy link
Contributor

Aight

@Harshal6927
Copy link
Contributor Author

I added return type because initializer of MakoTemplateEngine class have None as an return type. It is for the constancy.

@peterschutt
Copy link
Contributor

@Harshal6927 why don't we broaden this PR out and annotate the return for all __init__() methods that need it, at least that way we are consistent everywhere:

image

@Harshal6927
Copy link
Contributor Author

@Harshal6927 why don't we broaden this PR out and annotate the return for all __init__() methods that need it, at least that way we are consistent everywhere:

image

Should I do it?

@peterschutt
Copy link
Contributor

Should I do it?

I think so, best to be consistent.

And pep recommended:

(Note that the return type of __init__ ought to be annotated with -> None. The reason for this is subtle. If __init__ assumed a return annotation of -> None, would that mean that an argument-less, un-annotated __init__ method should still be type-checked? Rather than leaving this ambiguous or introducing an exception to the exception, we simply say that __init__ ought to have a return annotation; the default behavior is thus the same as for other methods.)

@Harshal6927
Copy link
Contributor Author

@peterschutt Ok I'll update my PR.

@jonadaly
Copy link
Contributor

jonadaly commented Sep 22, 2022

You missed a few __init__ definitions - could you do a quick search through the project for def __init__( and check you have got them all?

e.g.

  • app.py line 180
  • asgi.py line 71
  • response.py line 41, 115

...etc (there are quite a few more)

@Goldziher
Copy link
Contributor

@all-contributors add @Harshal6927 code

@allcontributors
Copy link
Contributor

@Goldziher

I've put up a pull request to add @Harshal6927! 🎉

@Goldziher Goldziher changed the title Added return type for initializer of DTOFactory class Update __init__ return typing to be consistent Sep 22, 2022
@Goldziher Goldziher merged commit 089153d into litestar-org:main Sep 22, 2022
Alex-CodeLab pushed a commit to Alex-CodeLab/starlite that referenced this pull request Sep 30, 2022
* Added return type for initializer of DTOFactory class

* Added return type for initializer

* code formatted correctly and ci.yaml changes

* Added return type for initializer
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

4 participants