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

Add support for streamlabs #427

Merged
merged 11 commits into from
Sep 18, 2021
Merged

Add support for streamlabs #427

merged 11 commits into from
Sep 18, 2021

Conversation

t-900-a
Copy link
Contributor

@t-900-a t-900-a commented Aug 15, 2021

Description:

Related issue (if applicable): #243

New Service Completion Status

  • apprise/plugins/NotifyStreamlabs.py
  • setup.py
    • add new service into the keywords section of the setup() declaration
  • README.md
    • add entry for new service to table (as a quick reference)
  • packaging/redhat/python-apprise.spec
    • add new service into the %global common_description

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • No lint errors (use flake8)
  • 100% test coverage

@t-900-a
Copy link
Contributor Author

t-900-a commented Aug 15, 2021

any recommendations to get the test coverage up?
I've only used
test_rest_plugins.py

Should I also create a
test_streamlabs.py

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2021

Codecov Report

Merging #427 (8ddf530) into master (836b729) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #427    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           95        96     +1     
  Lines        12083     12248   +165     
  Branches      2034      2060    +26     
==========================================
+ Hits         12083     12248   +165     
Impacted Files Coverage Δ
apprise/plugins/NotifyStreamlabs.py 100.00% <100.00%> (ø)
apprise/plugins/NotifyXML.py 100.00% <0.00%> (ø)
apprise/plugins/NotifyJSON.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 836b729...8ddf530. Read the comment docs.

@caronc
Copy link
Owner

caronc commented Aug 15, 2021

This is amazing; thank you very much for the PR! 🚀 ❤️ 🙂

any recommendations to get the test coverage up?

You should be able to probably cover all of your code using the test_rest_plugins.py. You're very close! You're just missing the tests where you get a non-200 response. There are lots of examples in the test_rest_plugins.py you should be able to copy/paste (near the end of each entry that test the Error Responses). That will probably bump you to 100%.

You can create an extra test file too if you want; but I usually only resort to them to test weird edge cases.

Chris

apprise/plugins/NotifyStreamlabs.py Outdated Show resolved Hide resolved
apprise/plugins/NotifyStreamlabs.py Outdated Show resolved Hide resolved
apprise/plugins/NotifyStreamlabs.py Outdated Show resolved Hide resolved
@caronc
Copy link
Owner

caronc commented Aug 20, 2021

@t-900-a I'm really impressed with this merge request, you've really done a great job. There are just a few small tweaks and I'll hit the merge:

  1. rename references to call to mode. That just makes it really consistent with all of the other Services that operate like this too. It would really make this flow nicely with them.
  2. Can you maybe rename the image_href and sound_href to image_url and sound_url. This is just another consistency thing to what Discord's plugin does
  3. I like to avoid underscores in general for ?kwargs= if i can.... so maybe change:
    • alert_type to just alert
    • special_text_color to just color
  4. Would you be able to just give me a quick blurb how you get the access token (to build the Apprise URL with)? Is it a token you just generate via the admin web area of Stream Labs? Or do you need to follow through with OAuth2 handshaking first to generate a key that expires eventually?

    I basically just need this info to help prepare a wiki page to document all of your awesome work. So any details /howto information would be super helpful! 🙂

btw: sorry to be such a pain; I'm a bit of a perfectionist, but your work is just fantastic - we're almost there

Edit: Just added the underscore point; please feel free to debate anything too if you disagree.
Edit: you can use the map_to keyword in the template definition map to keep the long underscored variable names for developer/code reading if you want. Its just the URLs i'm trying to simplify

@t-900-a
Copy link
Contributor Author

t-900-a commented Aug 22, 2021

1. rename references to `call` to `mode`.  That just makes it really consistent with all of the other Services that operate like this too. It would really make this flow nicely with them.

ok, will do

2. Can you maybe rename the `image_href` and `sound_href` to `image_url` and `sound_url`.  This is just another consistency thing to [what Discord's plugin does](https://github.com/caronc/apprise/blob/bb7c77105e050f586de76434d70647a573e01932/apprise/plugins/NotifyDiscord.py#L127)

ok, will do

3. I like to avoid underscores in general for `?kwargs=` if i can.... so maybe change:
   
   * `alert_type` to just `alert`
   * `special_text_color` to just `color`

ok, will do

4. Would you be able to just give me a quick blurb how you get the access token (to build the Apprise URL with)?  Is it a token you just generate via the admin web area of Stream Labs?  Or do you need to follow through with OAuth2 handshaking first to generate a key that expires eventually?  I basically just need this info to help prepare a wiki page to document all of your awesome work. So any details /howto information would be super helpful! 🙂

Documentation will be similar to this document, except we manually have to modify a url to retrieve the token.
https://github.com/Fittiboy/lnbits/blob/StreamAlerts/lnbits/extensions/streamalerts/README.md

Do you do the documentation all yourself, or do you want contributors? I'm not eager to write docs, but I can try to find someone to help.

For consistency to the existing code base I'm happy to make the changes. Expect some delay however.

@caronc
Copy link
Owner

caronc commented Aug 23, 2021

@t-900-a Great! thank you so much!

Documentation will be similar to this document, except we manually have to modify a url to retrieve the token.
https://github.com/Fittiboy/lnbits/blob/StreamAlerts/lnbits/extensions/streamalerts/README.md

This URL doesn't appear to work for me.

Do you do the documentation all yourself, or do you want contributors? I'm not eager to write docs, but I can try to find someone to help.

I don't mind putting together the wiki page. 🙂 I just need an 'explain it like I'm 5' example (or a link to another web page as your attempting to share too is fine) that lets me (and others) know how they can get an API token.

For consistency to the existing code base I'm happy to make the changes. Expect some delay however.

No rush at all; I really appreciate your efforts and great work! Thanks again ❤️ 👍

@t-900-a
Copy link
Contributor Author

t-900-a commented Aug 24, 2021

Working link: https://github.com/Fittiboy/lnbits/tree/master/lnbits/extensions/streamalerts#stream-alerts

Instructions will be a variation of the above.

@caronc
Copy link
Owner

caronc commented Sep 6, 2021

I started this wiki page sourcing the website you provided and attempted to give full credit to the images used. However i'm confused how your version of it with Apprise works since the example uses a client/secret combination, not an `api_token?

@t-900-a
Copy link
Contributor Author

t-900-a commented Sep 6, 2021

I started this wiki page sourcing the website you provided and attempted to give full credit to the images used. However i'm confused how your version of it with Apprise works since the example uses a client/secret combination, not an `api_token?

There's one additional step to get the api token.

I can add it to the wiki.

@caronc
Copy link
Owner

caronc commented Sep 18, 2021

@t-900-a: I'm going to do another version. I'd like to pack all of your efforts into it. So i'm going to merge your code. But it would be most appreciative if you could share the step(s) i'm missing to get the API key (in order to make everything you shared work).

@caronc caronc merged commit 5badfad into caronc:master Sep 18, 2021
@t-900-a
Copy link
Contributor Author

t-900-a commented Sep 19, 2021

@t-900-a: I'm going to do another version. I'd like to pack all of your efforts into it. So i'm going to merge your code. But it would be most appreciative if you could share the step(s) i'm missing to get the API key (in order to make everything you shared work).

Added a couple steps for the user to generate their access token. It is involved, but it's a one time deal.
https://pastebin.com/UiZ2fXxM

Thanks for merging!

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.

3 participants