-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
ref(typing): add typing to search utils file #40855
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
Conversation
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.
This is pretty awesome, nice work so far.
A question though: you have a bunch of # type: ignore[...]
lines in the code. There are some of them that don't seem to be because other modules are untyped.
For example, return euser.tag_value # type: ignore
makes sense to me, since the eventuser.py
file is untyped currently.
However there are other lines that seem like they should work, and I don't know that ignoring them is correct. For example, why is this a typing error: organization_id = project.organization_id # type: ignore[union-attr]
?
src/sentry/search/utils.py
Outdated
@@ -673,10 +711,10 @@ def parse_query(projects, query, user, environments): | |||
|
|||
results["query"] = " ".join(results["query"]) | |||
|
|||
return results | |||
return results # type: ignore[return-value] |
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 added a type for the return value, and then you are ignoring it here.
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.
Yeah, I get this Incompatible return value type (got "Dict[str, Any]", expected "ParsedQueryValues")
which makes sense since Dict[str, Any]
is more ambiguous than ParsedQueryValues(TypedDict)
Would casting the generic dict to ParsedQueryValues
make sense here?
# TODO(dcramer): handle query being wrapped in quotes | ||
tokens = tokenize_query(query) | ||
|
||
results = {"tags": {}, "query": []} | ||
results: dict[str, Any] = {"tags": {}, "query": []} |
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 would change this to results: ParsedQueryValues = ...
. Then you shouldn't need the cast
below.
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.
Hmm, spent some time trying to figure this out but there seems to be some type-checking incompatibilities with dict
and TypedDict
. Apparently you can't call update
on a TypedDict since its supposed to be immutable.
But then we go ahead and mutate the parsed query values here:
sentry/src/sentry/api/endpoints/group_events.py
Lines 126 to 127 in 74a5f38
query_kwargs = parse_query([group.project], raw_query, request.user, environments) | |
query = cast(str, query_kwargs.pop("query", None)) |
Looks like my usage of TypedDict
here is incorrect. Hmm. I think I'll have to change this back to return a regular old dict and comment on the typing in the method. What do you think?
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.
Looks like this is kind of a known bug in mypy: python/mypy#6462. I don't think it's about mutability, it's more .update
isn't type safe. It might be better to just add an ignore to the update line?
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.
Yeah, I can see why updating the TypedDict could be type unsafe. What if you .pop
a required k-v from the dict, now you're passing around a bad TypedDict. Or you .update
with a different value not specified in the typing.
I've swapped out the TypedDict usage with a general Dict and just added a docstring detailing what can be in the return dict.
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.
Well done!
Tried my best to try to inspect the actual types here.