caronc / apprise

Apprise - Push Notifications that work with just about every platform!
https://hub.docker.com/r/caronc/apprise
BSD 2-Clause "Simplified" License
11.7k stars 410 forks source link

Revolt Support #1057

Closed ktwrd closed 7 months ago

ktwrd commented 7 months ago

Description:

Revolt Apprise Support Added

Setup

New Service Completion Status

Checklist

Testing

I am unable to test this myself since my account got terminated a few months ago, but I know the API like it's the back of my hand. (Don't ask questions why, stay on-topic to the PR please)

Anyone can help test this source code as follows:

# Create a virtual environment to work in as follows:
python3 -m venv apprise

# Change into our new directory
cd apprise

# Activate our virtual environment
source bin/activate

# Install the branch
pip install git+https://github.com/ktwrd/apprise.git@revolt-support

# Test out the changes with the following command:
apprise -t "Test Title" -b "Test Message" \
  "revolt://bot_token/channel_id"
caronc commented 7 months ago

This is amazing! Thank you for this PR! I'll do a better review of it tomorrow, but just at a glance through my phone, it seems you did everything! 🚀🚀🙏🙏🎉🎉

Very impressive!

ktwrd commented 7 months ago

This is amazing! Thank you for this PR! I'll do a better review of it tomorrow, but just at a glance through my phone, it seems you did everything! 🚀🚀🙏🙏🎉🎉

Very impressive!

Thanks!

I'm a wee bit surprised that the unit testing worked as intended (copy & pasted from discord but removed markdown testing) since it's my first time contributing to a python project in quite a while.

I didn't implement sending attachments because the API is a bit weird for that and afaik Revolt's Websockets (which is still WIP) doesn't support that, and only bot/user accounts support it.

It would be a bit annoying to refer to the attachment documentation since that's not really a thing, and the only way I figured it out when I was making my C# library a while ago, was to look at the source code for autumn.

caronc commented 7 months ago

seems a couple of test cases are failing, i can help have a peak with you tomorrow.

codecov-commenter commented 7 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e9beea2) 99.27% compared to head (4a65913) 99.26%.

Files Patch % Lines
apprise/plugins/NotifyRevolt.py 97.69% 1 Missing and 2 partials :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1057 +/- ## ========================================== - Coverage 99.27% 99.26% -0.02% ========================================== Files 135 136 +1 Lines 17603 17733 +130 Branches 3593 3616 +23 ========================================== + Hits 17475 17602 +127 - Misses 119 120 +1 - Partials 9 11 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

caronc commented 7 months ago

@ktwrd ; i made some small changes to grant you 100% test coverage. The only thing i can't confirm is if it works. i presume it works for you when you try it out in a development environment?

I notice you have:

    # The 2000 characters above defined by the body_maxlen include that of the
    # title.  Setting this to True ensures overflow options behave properly
    overflow_amalgamate_title = True

Is this your intention; this was an edge case for Telegram, but didnt' apply to many other plugins. I'm guessing it impacts you too? Hence 2000 characters is the limit, no matter what... if a title exists of 1000 characters (for whatever reason), you will only have 1000 characters left to provide with the body? I only ask because i can see in your payload the title and body are in different parts of the payload... - just confirming#1057

ktwrd commented 7 months ago

@ktwrd ; i made some small changes to grant you 100% test coverage. The only thing i can't confirm is if it works. i presume it works for you when you try it out in a development environment?

I notice you have:

    # The 2000 characters above defined by the body_maxlen include that of the
    # title.  Setting this to True ensures overflow options behave properly
    overflow_amalgamate_title = True

Is this your intention; this was an edge case for Telegram, but didnt' apply to many other plugins. I'm guessing it impacts you too? Hence 2000 characters is the limit, no matter what... if a title exists of 1000 characters (for whatever reason), you will only have 1000 characters left to provide with the body? I only ask because i can see in your payload the title and body are in different parts of the payload... - just confirming#1057

I was unsure how to implement this (since I copy & pasted a lot from Discord) but revolt does have a 100 character limit for embed titles.

caronc commented 7 months ago

Perfect, fixed in last commit for you! body_maxlen of 2000 is okay as well?

I'll let you test out your configuration, the easiest way to do it is via Docker (see here) but basically:

docker-compose run --rm test.py311 bash

# inside container
bin/apprise -b "test" -vv revolt://credentials 

# boom; see if it works for you :)

Edit: @ktwrd Hence please confirm to me this works for you for the final merge

ktwrd commented 7 months ago

@caronc It works! screenshot

caronc commented 7 months ago

Merged! 🙏🚀