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

feat: Improve type annotations in utils.py #413

Merged
merged 3 commits into from Jul 5, 2019

Conversation

bluetech
Copy link
Contributor

@bluetech bluetech commented Jul 5, 2019

Replace some Anys with proper types.

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

Thanks for your continued efforts on this @bluetech!

@bluetech
Copy link
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
Member

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

@@ -358,7 +358,10 @@ def safe_repr(value):


def filename_for_module(module, abs_path):
# type: (str, str) -> str
# type: (str, Optional[str]) -> Optional[str]
Copy link
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
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 merged commit d460727 into getsentry:master Jul 5, 2019
@untitaker
Copy link
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.

None yet

2 participants