-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ref(analytics): Analytics Refactor + Types #30555
Conversation
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
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.
Just a couple small things. I'll leave approval to someone who understands better the semantic changes here.
@@ -1,3 +1,4 @@ | |||
from sentry.utils.imports import import_submodules | |||
|
|||
import_submodules(globals(), __name__, __path__) | |||
path = __path__ # type: ignore |
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.
Why did you need this change?
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.
Without the ignore
, mypy says:
error: Name '__path__' is not defined
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.
Got it.
|
||
if isinstance(value, dict): | ||
# Ensure we don't mutate the original. We do not need to deepcopy; | ||
# if it recurses into another Map it will once again copy itself. |
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.
You're aware that if the dictionary has a list or some other complex object in it, that object will not be copied? The comment is a little unclear here, I can't tell if you're saying copy
will recurse (which it won't) or extract
recurses.
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 just copied this class to a file so I don't know what the intention was for this comment. I'll leave a note in the code here.
0af18ac
to
436a10e
Compare
436a10e
to
ee91c62
Compare
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.
Lots of questions (just for my understanding) before I approve, looks good though!
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.
Thanks for helping me understand the changes, looks good to me 👍
This PR refreshes the analytics modules with a few things:
If there's too much going on here I think I can pull types into a separate PR.