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 type hints to sygnal/utils.py #276

Merged
merged 7 commits into from
Nov 22, 2021
Merged

Add type hints to sygnal/utils.py #276

merged 7 commits into from
Nov 22, 2021

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Nov 22, 2021

As the title states.

@H-Shay H-Shay requested a review from a team as a code owner November 22, 2021 19:34
@H-Shay H-Shay removed the request for review from a team November 22, 2021 19:35
@H-Shay H-Shay marked this pull request as draft November 22, 2021 19:35
@H-Shay H-Shay marked this pull request as ready for review November 22, 2021 19:58
@H-Shay H-Shay requested a review from a team November 22, 2021 19:59
@@ -65,7 +71,7 @@ def glob_to_regex(glob):
return re.compile(r"\A" + res + r"\Z", re.IGNORECASE)


def _reject_invalid_json(val):
def _reject_invalid_json(val: Any) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also use NoReturn here, but I don't think there's any practical benefit to doing so.

sygnal/utils.py Outdated
return f"[{self.extra['request_id']}] {msg}", kwargs


def glob_to_regex(glob):
def glob_to_regex(glob: str) -> Union[Pattern, Pattern[str]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the return type here---why can't we just say it returns a Pattern[str] in all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I nicked this from somewhere and now I can't remember where, but I think you are correct we can use the narrower case. I will change this.


async def twisted_sleep(delay, twisted_reactor):

async def twisted_sleep(delay: float, twisted_reactor: "SygnalReactor") -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine as it is, but I wanted to mention a couple of alternatives here. One be to write twisted_reactor: IReactorTime and only import IReactorTime; a second would be to move SygnalReactor into a new file, say sygnal/types.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I appreciate knowing alternative solutions, it really helps to have more tools in my arsenal.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

I'm happy if mypy is!

@H-Shay H-Shay merged commit 4a7367e into main Nov 22, 2021
@H-Shay H-Shay deleted the type_hints_utils.py branch November 22, 2021 21:44
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.

2 participants