-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update use of Slack API in qcodes.utils.slack.Slack class #2813
fix: update use of Slack API in qcodes.utils.slack.Slack class #2813
Conversation
Slack deprecated some of its API, which breaks most of the methods in the qcodes.utils.slack.Slack class. Update requests to Slack API in this class This commit closes issue microsoft#2806
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! Just to be sure, this has been tested, right?
the slack module actually has tests that are implemented via a mock, so those could be updated - do you intend to do that? |
…lack-sdk to setup.cfg; made edits to CONTRIBUTION.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for taking the time with the tests!
Yesterday, I tested the methods manually, against the Slack API and a bot and things work fine. Today, I started on the unit tests, but coverage of the class was something like 30%. Now it's 42% but I'm getting the hang of it :). I'll take a bit more time to finish the unit tests and try one last time against the bot we have in our system. |
Codecov Report
@@ Coverage Diff @@
## master #2813 +/- ##
==========================================
+ Coverage 64.85% 65.28% +0.42%
==========================================
Files 203 203
Lines 27840 27839 -1
==========================================
+ Hits 18056 18175 +119
+ Misses 9784 9664 -120 |
….txt; added two asserts to test_slack.py
…/deprecated-slack-api-breaks-slack-class
Hey @astafan8, I have added quite some unit tests, increasing coverage of qcodes.utils.slack from 30% to 95%. I will leave the remainder for the next person :). The new Slack class works on our system and the pipeline passes. Please let me know if you need anything more. Otherwise, as far as I'm concerned it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haroldmeerwaldt thank you so much for such a "clean" contribution! thanks for taking care of tests! this is a great contribution!
@astafan8 You're welcome! We use Qcodes quite a lot, so it's nice to contribute as well. |
Slack deprecated some of its API, which breaks most of the methods in
the qcodes.utils.slack.Slack class. Update requests to Slack API in this
class
Fixes #2806.
Changes proposed in this pull request:
Still to do:
@astafan8