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

Support Matrix Notice Message Types #460

Merged
merged 1 commit into from
Oct 25, 2021
Merged

Conversation

caronc
Copy link
Owner

@caronc caronc commented Oct 17, 2021

Description:

Related issue (if applicable): #458

The ability to add ?msgtype=notice to the matrix:// URL and create a m.notice instead of the default m.text.

The only supported types you can specify on the new msgtype enhancement is text or notice. By default text is implied, so there should be no visible changes to existing users already leveraging this plugin.

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

Testing

Anyone can help test this source code as follows:

# Create a virtual environment to work in
# This way you can just destroy it after when it's all over.
# The below will create a directory called apprise
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/caronc/apprise.git@458-matrix-notice-msgtype

# A message type set to `notice`
apprise -b "test" "matrix://user:[email protected]#channel/?msgtype=notice"

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2021

Codecov Report

Merging #460 (149a87b) into master (e2ebdbd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #460   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           97        97           
  Lines        12574     12585   +11     
  Branches      2134      2136    +2     
=========================================
+ Hits         12574     12585   +11     
Impacted Files Coverage Δ
apprise/plugins/NotifyMatrix.py 100.00% <100.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 e2ebdbd...149a87b. Read the comment docs.

Copy link

@HarHarLinks HarHarLinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from below comment, i.e. used correctly, it works great for me.

apprise/plugins/NotifyMatrix.py Show resolved Hide resolved
@caronc
Copy link
Owner Author

caronc commented Oct 24, 2021

I looked back into this and I do provide a warning. I noticed that you're using apprise in your example without any -v. Even just one more -v will increase the verbosity and give you more details. The idea is to keep Apprise relatively quiet without any specified verbosity (like running in silent mode by default).

It's a bit of a grey area to change this specific check from a WARNING to an ERROR (so it shows up always). If i do that, it would be inconsistent with all of the other plugins. But i could see the point in having it be an error (and thus updating all other plugins to report the same error as well (instead of a warning).

@HarHarLinks
Copy link

HarHarLinks commented Oct 24, 2021

Yes, I saw the warning in the code and wondered why it wasn't triggered. I tried with one -v as you suggest and indeed now it shows the warning in addition to the above error lines.

Maybe you can see why I am confused: The error says I provided no valid matrix URL which makes it sound like it's quoted wrong or uses illegal characters etc when actually the cause is known to be the invalid msgtype, it just won't tell you unless you known you gotta ask. From a UX perspective that should printed right from the start and spare everyone a lot of headache.

@caronc
Copy link
Owner Author

caronc commented Oct 25, 2021

I agree; it is confusing.

What I'll do is merge this code since it achieves what it was intended to do. But I created a new story to address this very concern you have (and be more wide-spread about it and make sure i get it everywhere). 👍

@caronc caronc merged commit 08b0577 into master Oct 25, 2021
@caronc caronc deleted the 458-matrix-notice-msgtype branch November 11, 2021 02:03
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