Skip to content

feat: Improve type annotations in utils.py#413

Merged
untitaker merged 3 commits into
getsentry:masterfrom
bluetech:typings-improve-utils
Jul 5, 2019
Merged

feat: Improve type annotations in utils.py#413
untitaker merged 3 commits into
getsentry:masterfrom
bluetech:typings-improve-utils

Conversation

@bluetech
Copy link
Copy Markdown
Contributor

@bluetech bluetech commented Jul 5, 2019

Replace some Anys with proper types.

Replace some Anys with proper types.
@untitaker
Copy link
Copy Markdown
Member

Thanks for your continued efforts on this @bluetech!

@bluetech
Copy link
Copy Markdown
Contributor Author

bluetech commented Jul 5, 2019

@untitaker This has one failure left:

sentry_sdk/utils.py:398: error: Argument 2 to "filename_for_module" has incompatible type "Optional[str]"; expected "str"

It looks legitimate but I'm not sure. The module variable in serialize_frame can be None, but filename_for_module doesn't except None. But filename_for_module is wrapped in a try...except Exception so it doesn't crash, but I'm not sure if that's intended.

Would you mind taking a look?

@untitaker
Copy link
Copy Markdown
Member

Yeah, seems like i was a bit careless about the logic on that one

Comment thread sentry_sdk/utils.py Outdated

def filename_for_module(module, abs_path):
# type: (str, str) -> str
# type: (str, Optional[str]) -> Optional[str]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking at serialize_frame, module can be None too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're right, not sure why mypy didn't catch that one. I think I fixed it now though!

@untitaker untitaker force-pushed the typings-improve-utils branch from d014f7e to fb66204 Compare July 5, 2019 14:45
@untitaker untitaker merged commit d460727 into getsentry:master Jul 5, 2019
@untitaker
Copy link
Copy Markdown
Member

Thanks again!

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