Skip to content

Conversation

ceciliaavila
Copy link
Collaborator

Fixes #1687

Description

This PR introduces the SkillConversationIdFactory class ported from DotNet, adding/updating the necessary functionality to support the new and legacy methods to be backward compatible.
New unit tests were added as well as updating the existing ones to support the new functionality.

Note: The class ConversationIdFactoryBase and file name were not updated to maintain parity from DotNet (SkillConversationIdFactoryBase) because it will represent a breaking change for anyone using this class.

Specific Changes

  • Added the new SkillConversationIdFactory class that inherits from ConversationIdFactoryBase with the new functionality.
  • Updated the ConversationIdFactoryBase class to deprecate the get_conversation_reference and introduce the get_skill_conversation_reference method.
    • @abstractmethod decorator was removed, so the user is not warned to implement the new method and the legacy one (Addressing a breaking change), the usage of the decorator was removed from the rest of the methods to maintain consistency, if any method is not implemented from the Base class it will throw a NotImplemented error.
  • Updated the SkillHandler class to support the new get_skill_conversation_reference method and be backward compatible with the legacy one
    • Also a default Logger was added when not provided to enable the use of the logger functionality inside the class (ported from DotNet).
  • Added new unit tests in the test_skill_conversation_id_factory.py file to support the new SkillConversationIdFactory class.
  • Updated test_skill_dialog.py and test_skill_http_client.py files to support the new SkillConversationIdFactory functionality instead of the legacy one.
  • Updated test_skill_handler.py to replace all the tests that were using the legacy functionality to start using the new one.
    • For legacy functionality a new test test_legacy_conversation_id_factory were ported from DotNet.

Testing

The following images show the test passing locally and in the CI pipelines.
image

@tracyboehrer
Copy link
Member

@ceciliaavila Sorry to ask for a branch update again. Each PR merge means the others need it again. There is another from @denscollo that we'll need to coordinate to save some cycles.

@tracyboehrer tracyboehrer merged commit 85ec376 into microsoft:main Jun 15, 2021
@ceciliaavila ceciliaavila deleted the southworks/update/skill-conversation-id-factory branch June 15, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants