Rsarkar/communication chat preview5 #17325
Conversation
sarkar-rajarshi
requested review from
a team,
ankitarorabit,
memontic-ms and
turalf
as code owners
ghost
added
the
There was a problem hiding this comment.
The snippets will need a bit more context than this (and this applies to other existing snippets in the readme).
The goal is that a user should be able to copy+paste the code and use it right away.
So something more like:
client = ChatClient(...)
thread = client.get_chat_thread_client(...)
thread.get_properties(...)Given that this applies to multiple snippets throughout the readme - I'm happy if you want to open a github issue to address it in a subsequent PR.
There was a problem hiding this comment.
Will update the README and the samples in a separate PR
There was a problem hiding this comment.
add_participants should be a no-op if passed an empty list, so a user should be able to do:
chat_thread_client.add_participants([p for p, error in create_chat_thread_result.errors if decide_to_retry(error)])Additionally, if you need to check that a list has 0 elements, you can just do this in python:
need_to_retry = [p for p, error in create_chat_thread_result.errors if decide_to_retry(error)]
if need_to_retry:
chat_thread_client.add_participants(need_to_retry)There was a problem hiding this comment.
add_participants has been updated to be no-op if passed empty list. Will update the README and the samples in a separate PR.
There was a problem hiding this comment.
Maybe "Gets the properties of the chat thread."?
There was a problem hiding this comment.
Under what circumstances is ValueError raised? Unless it's something specific that users need to be aware of and code for, we can just remove this.
There was a problem hiding this comment.
It was a miss. It does not throw ValueError in any scenario. This sneaked in when transferring the method from chat_client :)
There was a problem hiding this comment.
I think the typehint should be something like:
from typing import List, Tuple
List[Tuple[ChatThreadParticipant, ChatError]]There was a problem hiding this comment.
I see this is what you already had in the docstring :return: value :)
There was a problem hiding this comment.
It used to show up as a flag in apiview before. Have refactored this
There was a problem hiding this comment.
I'm not sure if this is a bug in autorest or swagger spec - but I think this should be returning an Iterable["_models.ChatThreadItem"].
It doesn't really matter in the generated code, so long as we've confirmed that the hand-written layer over the top is doing the right thing :)
There was a problem hiding this comment.
This is an autorest thing. It is exposed as ItemPaged[ChatThreadItem] to the outside world.
There was a problem hiding this comment.
I believe we refactored out this CommunicationTokenRefreshOptions object
There was a problem hiding this comment.
yes. The changes were in a dangling state in a separate PR #17178 . It's been merged.
There was a problem hiding this comment.
The credentials objects should just be imported from azure.communication.chat now right?
There was a problem hiding this comment.
yes. The changes were in a dangling state in a separate PR #17178 . It's been merged.
There was a problem hiding this comment.
Why is this marked live test only?
There was a problem hiding this comment.
Why do we need to use mock responses here? Is there some reason the recording wont work?
|
/azp run python - communication - tests |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
sarkar-rajarshi
changed the base branch from
feature/communication-chat-preview5
to
master
|
/azp run python - communication - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* make CommunicationUserIdentifierSerializer private * CommunicationTokenCredential + AccessToken fix - Initialize CommunicationTokenCredential with token and not with CommunicationTokenRefreshOptions - Convert AccessToken.expires_on to int as compared to datetime * add token_refresher to CommunicationTokenCredential * update docstring * Identifier fields renaming _ credential import fixes in sample code
- CommunicationError renamed to ChatError
- Rename method - Move get_properties from ChatClient to ChatThreadClient - Change method signature - remove parameter thread_id - Modify unit and e2e tests - move tests from chat_thread_* to chat_thread_client_* - Update README.md
- Changes made due to flattening of CreateChatThreadResult and AddChatParticipantsResult
|
/azp run python - communication - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
3 participants
No description provided.