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

fix(types): Replace custom TYPE_CHECKING with stdlib typing.TYPE_CHECKING #3447

Merged
merged 5 commits into from
Aug 27, 2024

Conversation

dev-satoshi
Copy link
Contributor

This commit replaces the custom TYPE_CHECKING constant in _types.py with the standard library's typing.TYPE_CHECKING. This change simplifies the code and removes unnecessary backward compatibility for Python versions below 3.6. Since the _types module is private, this update does not introduce any breaking changes.

Closes #3426

@sentrivana
Copy link
Contributor

Hey @dev-satoshi, thank you for the quick PR!

This works, but I'd prefer if we could remove the TYPE_CHECKING constant from _types.py completely and update all our usages of it in the code to instead import TYPE_CHECKING from typing. A quick search for from sentry_sdk._types import TYPE_CHECKING should give you all the occurrences (heads up, it's quite a lot).

@dev-satoshi
Copy link
Contributor Author

@sentrivana
Got it! I’ll take care of it!

Copy link
Contributor Author

@dev-satoshi dev-satoshi left a comment

Choose a reason for hiding this comment

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

@sentrivana
It's done!
Could you please review this when you have a moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two import statements in this file, so it's better to remove one of them, right?

try:
from typing import TYPE_CHECKING
except ImportError:
TYPE_CHECKING = False
from typing import TYPE_CHECKING
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 removed the try-except block since we are now supporting Python 3.6 and above.

Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Thank you @dev-satoshi! Looks good to me :)

@sentrivana sentrivana added the Trigger: tests using secrets PR code is safe; run CI label Aug 19, 2024
Copy link

codecov bot commented Aug 19, 2024

❌ 4 Tests Failed:

Tests completed Failed Passed Skipped
110 4 106 0
View the top 3 failed tests by shortest run time
tests.integrations.aws_lambda.test_aws�test_init_error[python3.9]
Stack Traces | 8.88s run time
.../integrations/aws_lambda/test_aws.py:334: in test_init_error
(event,) = envelope_items
E ValueError: not enough values to unpack (expected 1, got 0)
tests.integrations.aws_lambda.test_aws�test_init_error[python3.11]
Stack Traces | 8.88s run time
.../integrations/aws_lambda/test_aws.py:334: in test_init_error
(event,) = envelope_items
E ValueError: not enough values to unpack (expected 1, got 0)
tests.integrations.aws_lambda.test_aws�test_init_error[python3.10]
Stack Traces | 8.95s run time
.../integrations/aws_lambda/test_aws.py:334: in test_init_error
(event,) = envelope_items
E ValueError: not enough values to unpack (expected 1, got 0)

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Aug 19, 2024
@sentrivana
Copy link
Contributor

Just fyi, I've pushed a commit that reorders some of the imports (e.g. if the typing import is in the middle of sentry_sdk imports, I move it directly to the if TYPE_CHECKING block). Will merge this as soon as the tests are happy.

@sentrivana sentrivana added the Trigger: tests using secrets PR code is safe; run CI label Aug 19, 2024
@sentrivana
Copy link
Contributor

All is looking good except for the AWS Lambda tests (this has nothing to do with this PR, we need to fix this on master). We'll probably get around to fixing this early next week and then we can merge this.

Nothing additional needed from your side @dev-satoshi -- thanks again for the contribution, we'll see that this is merged next week!

@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Aug 27, 2024
@sentrivana sentrivana enabled auto-merge (squash) August 27, 2024 12:30
@sentrivana sentrivana merged commit 4b361c5 into getsentry:master Aug 27, 2024
122 of 124 checks passed
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.

Get rid of sentry_sdk._types.TYPE_CHECKING
2 participants