Mypy Compatibilty for EventGrid #14344
Conversation
ghost
added
the
|
/azp run python - eventgrid - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventgrid - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
rakshith91
requested review from
danieljurek,
KieranBrantnerMagee,
lmazuel,
mitchdenny,
scbedd and
weshaggard
as code owners
rakshith91
changed the title
sdk/eventgrid/azure-eventgrid/azure/eventgrid/_signature_credential_policy.py
Show resolved
Hide resolved
| @@ -35,7 +35,7 @@ def decode_cloud_event(self, cloud_event, **kwargs): # pylint: disable=no-self-u | |||
| cloud_event = CloudEvent._from_json(cloud_event, encode) # pylint: disable=protected-access | |||
| deserialized_event = CloudEvent._from_generated(cloud_event) # pylint: disable=protected-access | |||
| CloudEvent._deserialize_data(deserialized_event, deserialized_event.type) # pylint: disable=protected-access | |||
| return deserialized_event | |||
| return cast(CloudEvent, deserialized_event) | |||
There was a problem hiding this comment.
What was the error that triggered the need for this cast? I would expect the return type of CloudEvent._from_generated to be CloudEvent , and then this cast unecessary
There was a problem hiding this comment.
This was unnecessary - removed it
| @@ -58,7 +58,7 @@ def decode_eventgrid_event(self, eventgrid_event, **kwargs): # pylint: disable=n | |||
| eventgrid_event = EventGridEvent._from_json(eventgrid_event, encode) # pylint: disable=protected-access | |||
| deserialized_event = EventGridEvent.deserialize(eventgrid_event) | |||
| EventGridEvent._deserialize_data(deserialized_event, deserialized_event.event_type) # pylint: disable=protected-access | |||
| return deserialized_event | |||
| return cast(EventGridEvent, deserialized_event) | |||
There was a problem hiding this comment.
this is necessary unlike above - we get
Returning Any from function declared to return "EventGridEvent"
because the msrest's deserialize method on line 59 isn't typed
There was a problem hiding this comment.
Then let's fix msrest too :)
Azure/msrest-for-python#226
But ok here, since we should not wait for msrest fix
|
(to be clear, obviously pending laurent's changes, but if you had an answer to those I didn't see anything else glaring) |
| @@ -82,7 +86,7 @@ def _get_authentication_policy(credential): | |||
| return authentication_policy | |||
|
|
|||
| def _is_cloud_event(event): | |||
| # type: dict -> bool | |||
| # type: (dict) -> bool | |||
There was a problem hiding this comment.
Input is actually Any, since since if the input is not a dict, you don't raise an exception, you return False.
This will remove the cast later down the road
There was a problem hiding this comment.
MY rationale was that we expect only a dictionary here - but yes, you are right - changing it
|
|
||
| if all(isinstance(e, CloudEvent) for e in events) or all(_is_cloud_event(e) for e in events): | ||
| if all(isinstance(e, CloudEvent) for e in events) or all(_is_cloud_event(cast(Dict, e)) for e in events): |
There was a problem hiding this comment.
cast(Dict, e) not necessary once you change _is_cloud_event input to Any
| elif all(isinstance(e, CustomEvent) for e in events): | ||
| serialized_events = [dict(e) for e in events] | ||
| self._client.publish_custom_event_events(self._topic_hostname, serialized_events, **kwargs) | ||
| serialized_events = [dict(e) for e in events] # type: ignore |
There was a problem hiding this comment.
Argument 1 to "dict" has incompatible type "Union[CloudEvent, EventGridEvent, CustomEvent, Dict[Any, Any]]"; expected "Mapping[Any, Any]"
I can use a cast, but it won't entirely be true - ideally we validate that it's not a cloudevent, eventgrid event by the time we hit this line and they should not be included in the union - afaik, it's a problem with mypy.
|
/azp run python - eventgrid - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…into add_business_multipage_tests * 'master' of https://github.com/Azure/azure-sdk-for-python: (24 commits) samples updates from other branch (Azure#14598) [form recognizer] add multipage business card form (Azure#14613) Remove extra newline at the end of the file (Azure#14608) Update .gitignore (Azure#14609) Enable the link check for link verification step. (Azure#14604) Switch the content from array to string. (Azure#14576) [Fileshare] Added support for set share properties including access tier (Azure#14355) [EventHubs & ServiceBus] add python3.9 support (Azure#14301) Add parse_key_vault_certificate_id method and tests (Azure#14518) Enable Codespaces. (Azure#14564) Failed the anchor links with Uppercase. (Azure#14535) Sync eng/common directory with azure-sdk-tools for PR 1091 (Azure#14550) Only check the touched markdown files in PR for the Verify link step (Azure#14466) [formrecognizer] add logic to set page_number on `ContactNames` field (Azure#14552) update deps for multiapi (Azure#14534) add sample tests for business cards and model compose (Azure#14515) Ma accept str for datetime (Azure#14517) Fix anchor links so they work when converting to html [formrecognizer] initial selection marks (Azure#14024) Mypy Compatibilty for EventGrid (Azure#14344) ...
Fixes #13883