Skip to content
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

Conversation

haroldmeerwaldt
Copy link
Contributor

@haroldmeerwaldt haroldmeerwaldt commented Mar 8, 2021

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:

  • requests to Slack API in qcodes.utils.slack.Slack

Still to do:

  • check if tests pass

@astafan8

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
Copy link
Contributor

@astafan8 astafan8 left a 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?

qcodes/utils/slack.py Outdated Show resolved Hide resolved
qcodes/utils/slack.py Show resolved Hide resolved
@astafan8
Copy link
Contributor

astafan8 commented Mar 8, 2021

check if tests pass

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
Copy link
Contributor

@astafan8 astafan8 left a 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!

CONTRIBUTING.rst Outdated Show resolved Hide resolved
@haroldmeerwaldt
Copy link
Contributor Author

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.

qcodes/tests/test_slack.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #2813 (89a8f18) into master (cf0e83a) will increase coverage by 0.42%.
The diff coverage is 100.00%.

@@            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     

@haroldmeerwaldt
Copy link
Contributor Author

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.

Copy link
Contributor

@astafan8 astafan8 left a 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 astafan8 merged commit ec9b835 into microsoft:master Mar 12, 2021
@haroldmeerwaldt
Copy link
Contributor Author

@astafan8 You're welcome! We use Qcodes quite a lot, so it's nice to contribute as well.

@haroldmeerwaldt haroldmeerwaldt deleted the hotfix/deprecated-slack-api-breaks-slack-class branch March 12, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slack API deprecating early methods breaks qcodes.utils.slack.Slack class
2 participants