-
Notifications
You must be signed in to change notification settings - Fork 543
ref: Improve scrub_dict typing #2768
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6d8cb89
ref: Improve scrub_dict typing
szokeasaurusrex e31a24f
Merge branch 'master' into szokeasaurusrex/scrubber-typing
szokeasaurusrex 83171ac
Removed redundant `isinstance` checks
szokeasaurusrex d1126c6
Merge branch 'master' into szokeasaurusrex/scrubber-typing
szokeasaurusrex 433e615
Clarifying comments
szokeasaurusrex ef8903f
Handle `cast` `ImportError`
szokeasaurusrex 133e8fa
Merge branch 'master' into szokeasaurusrex/scrubber-typing
szokeasaurusrex File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get where you're coming from (the method theoretically does support any type), but as a user I'd find it confusing that a method called
scrub_list
accepts an argumentlst
of any type.Since types are not enforced at runtime, users are free to ignore the type hint that says a
List[Any]
should be given to this method, which is why theisinstance(lst, list)
check is there now -- just to make sure we don't explode if someone misuses the function. I feel like by changing the type hint to say this function supports anyobject
, we're making it harder for users to understand how this method is meant to be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need this change for the Event TypedDict changes (#2753). Originally, I was going to include these changes there, but I split these into a separate PR, since they are not strictly related.
We should use the
object
type here because without this change, thesentry_sdk.scrubber.EventScrubber.scrub_request
method raises type errors on the #2753 code. The reason is thatevent["request"]
has a type ofdict[str, object]
, since the request object (at least from my understanding) can contain an arbitrary mapping from strings to any object. We are then callingscrub_dict
on several objects withinevent["request"]
(such asevent["request"]["headers"]
), which according to the type checker have typeobject
. However without this PR, thescrub_dict
method is declared as only acceptingdict[str, Any]
, so the type checker fails, since based on the type hint it appears that the code is not type safe.However, because of the fact that we are doing the
isinstance
check within thescrub_dict
method, the code is in fact type safe. We can pass anyobject
toscrub_dict
and the code will behave correctly.I understand your point, but there is already a doc comment here to explain that the method only does anything if the argument passed is a list, and I will also add a similar comment to the
scrub_dict
method to clarify.I prefer
object
here, since in my opinion, the purpose of type hints is to communicate to users what type they need to pass in order for type safety to be ensured. For these methods, users can pass objects even with an unknown type without violating the method contract, and type safety is always guaranteed. Going back to thescrub_request
example that prompted me to make this change, having typeobject
makes it clear that I can safely passevent["request"]["headers"]
without first adding anisinstance
check to make sure thatevent["request"]["headers"]
is a dictionary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha -- then let's go with your change.