search

rveachkc / pymsteams

Format messages and post to Microsoft Teams.
https://pypi.org/project/pymsteams/
Apache License 2.0
424 stars 78 forks source link

Implement async send #105

Closed nickolay-github closed 2 years ago

nickolay-github commented 3 years ago

Hi! Thanks for the helpful library!

In the PR I added an implementation of the asynchronous send function

As I understood, having seen these tickets (#71 #84 ), this feature would be useful)

I will be glad to receive comments and remarks!

rveachkc commented 3 years ago

I'm going to need some time to review this, as I'm not familiar with httpx. Truthfully, I'd prefer an async send method that kept with the standard library+requests, but I'll do some research on that.

Additional thoughts:

nickolay-github commented 3 years ago

Okay, got it.

It seems to me that, unfortunately, the standard python+requests library is not enough. The requests library does not support asynchronous calls. I chose httpx, mainly because the library is very similar to requests, only with asynchronous: method signatures and API are compatible with requests and there is an indication in the documentation of the original requests that httpx is a good asynchronous alternative.

rveachkc commented 3 years ago

If this is going to be added to the setup.py, I'd really prefer httpx be installed if somebody installs with a pip install pymsteams[async]. That way, if somebody still needs to install on py2, it won't break.

This library uses the actionable message card api, which uses the Legacy actionable message format.

The first note for this reference is:

This document describes the original JSON format for the actionable message card format. For actionable messages sent via email, this has been replaced with the Adaptive Card format. Microsoft recommends that new actionable message integrations use the Adaptive Card format, and existing integrations consider updating to Adaptive Card format. The Adaptive Card format is required to support Outlook on iOS and Android. However, if you are sending actionable messages via an Office connector, or to a Microsoft Teams connector, you must continue to use the message card format.

I'd like to eventually work in code to use this new format instead: https://docs.microsoft.com/en-us/outlook/actionable-messages/adaptive-card

nickolay-github commented 2 years ago

Added an additional option to the setup.py for install the asynchronous version

rveachkc commented 2 years ago

I like this. Thanks for submitting.

I can check this in a couple days. Should it get merged, it'll probably be released as a version 0.2.0.

maslyankov commented 2 years ago

Any update on merging this?

rveachkc commented 2 years ago

Apologies for the delay. Between the holidays and fitness goals, I've had limited personal time until recently.

I'm wanting to make a couple changes before merging to master, so I created a new branch, and I'm working on those in #116. Since async sending is a fairly significant change, this will be released as version 0.2.0.

I would like to comment on two things about this PR that really delayed my review. Had that not been the case, it probably would have been merged to master and released much sooner.

No documentation was included in the pull request

This needs to be done before this is released, and it would have been much easier to review had that been done. Honestly, I don't mind learning about httpx and asyncio, but I have zero need for it, so I had to dedicate some time to sit down and learn it.

Style and formatting updates included in the initial commit.

Honestly, when I first read through this PR, I got frustrated needing to review many non-functional changes alongside the actual enhancement.

I understand....my formatting was not ideal, but in the future, please separate style and formatting changes from the functional one.

Ideally, I'd like them in separate PR's, but at minimum, push them in separate commits.