Fix mypy and pylint errors for communications chat and rooms#43283
Fix mypy and pylint errors for communications chat and rooms#43283natekimball-msft wants to merge 1 commit intomainfrom
Conversation
natekimball-msft
requested review from
a team,
LuChen-Microsoft,
maximrytych-ms and
mjafferi-msft
as code owners
natekimball-msft
requested review from
jakublehotsky,
stefang931 and
thisisjasleen
There was a problem hiding this comment.
Pull Request Overview
This PR addresses mypy and pylint errors in the Azure Communication Chat and Rooms SDKs by replacing legacy type hints with modern type annotations and adding proper type ignores for known type system limitations.
Key Changes
- Converted legacy comment-style type hints (
# type:) to modern Python type annotations - Added
type: ignorecomments for mypy compatibility issues where types don't align perfectly - Updated function signatures to use keyword-only parameters where appropriate
- Fixed version extraction logic in setup.py files to handle potential None values
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py files | Fixed version extraction to handle None regex matches properly |
| _shared/utils.py files | Updated type hints for token credential functions |
| Client files | Modernized type annotations and added mypy ignore comments |
| _utils.py | Updated type annotations for error response converter |
| _communication_identifier_serializer.py | Added type annotations and ignore comments |
| _shared/token_utils.py | Added ignore for untyped import |
Comments suppressed due to low confidence (1)
| from datetime import datetime, timezone | ||
| from typing import Tuple, Any, List, Optional | ||
| import isodate | ||
| import isodate # type: ignore[import-untyped] |
There was a problem hiding this comment.
[nitpick] Consider adding a py.typed file to the isodate package or use a typed alternative if available, rather than ignoring the untyped import.
| create_room_request["participants"] = { # type: ignore[assignment] | ||
| p.communication_identifier.raw_id: {"role": p.role} for p in participants | ||
| } |
There was a problem hiding this comment.
[nitpick] The type ignore comment suggests a type system issue. Consider defining a proper TypedDict for the participants structure to improve type safety.
| create_room_request["participants"] = { # type: ignore[assignment] | ||
| p.communication_identifier.raw_id: {"role": p.role} for p in participants | ||
| } |
There was a problem hiding this comment.
[nitpick] The type ignore comment suggests a type system issue. Consider defining a proper TypedDict for the participants structure to improve type safety.
annatisch
left a comment
annatisch
left a comment
There was a problem hiding this comment.
Thanks for these updates!
There are a few typing errors here that surprise me, and I would like to dig into it a little more to find out why mypy was complaining.
| # pylint: disable=unused-import,ungrouped-imports | ||
| from datetime import datetime | ||
| from azure.core.paging import ItemPaged | ||
| pass |
There was a problem hiding this comment.
Rather than pass here, we can just remove the whole TYPE_CHECKING block (and remove the TYPE_CHECKING import on line 7).
| # type: (...) -> ChatThreadClient | ||
| thread_id: str, | ||
| **kwargs: Any | ||
| ) -> "ChatThreadClient": |
There was a problem hiding this comment.
| ) -> "ChatThreadClient": | |
| ) -> ChatThreadClient: |
Shouldn't need quotes here.
| ): | ||
| # type: (...) -> None | ||
| endpoint: str, | ||
| credential: "CommunicationTokenCredential", |
There was a problem hiding this comment.
| credential: "CommunicationTokenCredential", | |
| credential: CommunicationTokenCredential, |
Shouldn't need quotes here.
| ) | ||
|
|
||
| return create_chat_thread_result | ||
| return create_chat_thread_result # type: ignore[return-value] |
There was a problem hiding this comment.
This shouldn't be a typing error - I think if you rename the variable on line 172 this (and the typing error on 172) should go away.
| start_time = kwargs.pop("start_time", None) | ||
|
|
||
| return self._client.chat.list_chat_threads(max_page_size=results_per_page, start_time=start_time, **kwargs) | ||
| return self._client.chat.list_chat_threads( # type: ignore[return-value] |
There was a problem hiding this comment.
hmmm this shouldn't be a typing error?
|
|
||
| def create_token_credential(): | ||
| # type: () -> FakeTokenCredential or get_credential | ||
| # type: () -> Any |
There was a problem hiding this comment.
I think we can just remove the old-style type hints completely from this file - there's not really much benefit to having the typing in test utils anyway.
|
Hi @natekimball-msft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
github-actions
bot
added
the
no-recent-activity
|
Hi @natekimball-msft. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
2 participants
Description
This PR is a follow up to the conversation in this PR: #43090 (comment)
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines