Skip to content

Add typing.Optional on missing annotations#1549

Merged
Kludex merged 5 commits intomasterfrom
chore/add-optional-to-none
Apr 21, 2022
Merged

Add typing.Optional on missing annotations#1549
Kludex merged 5 commits intomasterfrom
chore/add-optional-to-none

Conversation

@Kludex
Copy link
Owner

@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 lovelydinosaur 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
Owner 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
Owner 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
Contributor

@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
Contributor

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.

3 participants