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 typing.Optional on missing annotations #1549

Merged
merged 5 commits into from Apr 21, 2022
Merged

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Mar 20, 2022

There was a previous comment by @tomchristie about adding typing.Optional where is missing in a single PR.

I can't find the comment.

Note: I need to add no-implicit-optional on mypy configuration.

@Kludex Kludex requested a review from tomchristie March 20, 2022 06:24
@odiseo0
Copy link

odiseo0 commented Mar 25, 2022

I have a question why use
import typing
instead of
from typing import Optional, Sequence, Callable
Is there a code style guide for contributing in Starlette?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Mar 25, 2022

I have a question why use
import typing
instead of
from typing import Optional, Sequence, Callable
Is there a code style guide for contributing in Starlette?

It's just to comply with the current pattern you see around in the code.

@odiseo0
Copy link

odiseo0 commented Mar 25, 2022

I have a question why use
import typing
instead of
from typing import Optional, Sequence, Callable
Is there a code style guide for contributing in Starlette?

It's just to comply with the current pattern you see around in the code.

Understood, thanks

@Kludex Kludex requested a review from a team April 11, 2022 07:12
@Kludex Kludex mentioned this pull request Apr 15, 2022
2 tasks
@Kludex
Copy link
Sponsor Member Author

Kludex commented Apr 21, 2022

I've added no_implicit_optional, so we don't have a future regression on this subject. I've also ignored this rule on testclient.py, as we are thinking about changing the implementation there.

@adriangb Does your approval stands after my latest changes?

@Kludex Kludex added feature New feature or request clean up Refinement to existing functionality labels Apr 21, 2022
@Kludex Kludex added this to the Version 0.19.1 milestone Apr 21, 2022
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.

Yep I think this is good. I gave this a scroll through, I didn't see anything wrong so if MyPy is happy, I'm happy. LGTM!

Comment on lines +8 to +12
no_implicit_optional = True
show_error_codes = True

[mypy-starlette.testclient]
no_implicit_optional = False
Copy link
Member

Choose a reason for hiding this comment

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

👍 good call

@Kludex Kludex merged commit 5b56e7d into master Apr 21, 2022
@Kludex Kludex deleted the chore/add-optional-to-none branch April 21, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Refinement to existing functionality feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants