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

APRS (Automated Packet Reporting System) Ham Radio plugin #1005

Merged
merged 49 commits into from
Dec 16, 2023
Merged

APRS (Automated Packet Reporting System) Ham Radio plugin #1005

merged 49 commits into from
Dec 16, 2023

Conversation

joergschultzelutter
Copy link
Contributor

@joergschultzelutter joergschultzelutter commented Nov 19, 2023

Description: APRS (Automated Packet Reporting System) Ham Radio plugin

Related issue (if applicable): not available; new plugin

  • Description and Constraints: see README_aprs.md
  • test coverage @ 66% - I cannot get the test_plugin_aprs_urls part to work (maybe I'm just plain stupid). All other tests seem to work fine.
  • The test script has a few sections which are commented out deliberately (lines 234ff. and 291ff.)
  • This plugin is tied to a so-called "device ID" which is (radio-)transmitted as part of the APRS message broadcast. Basically, the device ID acts as an identification of the device/software/... which was used for generating that message. This device ID is tied to my credentials as you are likely not a ham radio amateur. Details: here. tl;dr: in case I messed up the plugin's code somehow, the APRS guys will get in touch with me and spare your life instead 😃

New Service Completion Status

  • apprise/plugins/NotifyAprs.py
  • KEYWORDS
    • new service plugin for sending APRS messages via Apprise.
  • README.md
  • 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 (applies to the actual plugin)
  • No lint errors (use flake8)
  • 100% test coverage (see Description)

Testing

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/caronc/apprise.git@<this.branch-name>

# Test out the changes with the following command:
apprise -b "Test Message" \
  -vv -b "Hello World from Apprise" "aprs://<APRS source callsign>:<APRS source callsign passcode>@<aprs target callsign>"

@joergschultzelutter
Copy link
Contributor Author

Note: this plugin uses unidecode as external dependency (and has been added to requirements.txt respectively). Theoretically, it IS possible to remove this dependency if you are not happy with including that package - but I'd rather keep that module included, thus being able to generate plain ASCII code (which is required for APRS) from whatever format/language the user may have entered. Nevertheless, the PR checks lack this module (and therefore failed to do their magic).

@caronc
Copy link
Owner

caronc commented Nov 21, 2023

I'll need to review this, hopefully this weekend.

Offhand, i would like:

  • to avoid the new requirements you added. It's a good idea, but not for a specific plugin. At best it should be a directive and applied to the body/title from the NotifyBase.py where by default it isn't applied, but can be turned off or on. I am not sure if it's needed at all personally.
  • drop the image from the library. Images can be attached to the ticket or pr like this one and then referenced through the wiki. This is where we can put all of that amazing work you did with your readme too. That can go into the wiki; it should not be part of the project.

Overall though, you did a nice job with the actual plugin. I can help with the test cases this weekend (hopefully).

@joergschultzelutter
Copy link
Contributor Author

I am with you on the additional plugin requirements issue - wasn't happy about this either. I have amended the code - which now detects if the additional package is installed or not (meaning that I reverted Apprise's requirements.txt file to its original setting). Last option would be a complete removal option of this text pre-processing and have it done outside of Apprise. Let me know whatever option you prefer.

I will also remove the images folder accordingly.

No rush with reviewing this PR.

@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2023

Codecov Report

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

Comparison is base (a25ff00) 99.51% compared to head (a47af1c) 99.52%.
Report is 2 commits behind head on master.

Files Patch % Lines
apprise/plugins/NotifyAprs.py 99.59% 0 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #1005    +/-   ##
========================================
  Coverage   99.51%   99.52%            
========================================
  Files         126      128     +2     
  Lines       16868    17126   +258     
  Branches     3457     3502    +45     
========================================
+ Hits        16787    17044   +257     
  Misses         70       70            
- Partials       11       12     +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@caronc
Copy link
Owner

caronc commented Dec 3, 2023

The only test cases i haven't added are your added requirements for unidecode; is this nessisary?

I would presume you could just perform all your unidecode calls before using the apprise library. Not sure at this point if this needs to be inside the apprise bundle itself?

Aside from that, i got rid of your time.sleep() and built your requested throttles into the apprise version of it. It should still behave the same way.

@caronc
Copy link
Owner

caronc commented Dec 3, 2023

I pushed 1 more commit that is easily rolled back to show your addition completed and ready for Apprise. Perhaps we could go with this, and do another release later one to improve your edge case character handling?

@joergschultzelutter
Copy link
Contributor Author

Sorry for my delayed response - I have been completely snowed-under at work. Please feel free to remove the 'unidecode' references from the code, if necessary.

The reason why I initially wanted to include that module in the plugin is the following:

Imagine that you use a single Apprise config file which contains two target messengers:

  • messenger A is a 'regular' messenger such as Telegram, Teams, etc and is capable of digesting UTF-8
  • messenger B is SMS-based and can only digest ASCII-7 bit

Now let's assume that you send a cyrillic UTF-8 message to both targets by using that config file. Messenger A is capable of digesting the message as is - but messenger B might choke on that message and/or its contents will trigger unknown side effects if it gets 'translated' to a control character.

Currently, the only workaround is to use two Apprise config files: one for regular messengers and one for plain-text messengers where the outgoing text can get preprocessed by the program that calls Apprise. I don't see any other option - unless the pre-processing of the text can be done inside of the plugin.

tl;dr: for now, feel free to get rid of the 'unidecode' package. For a future Apprise version, running an internal UTF-to-plain-ASCII-conversion for those messengers not capable of sending UTF-8 content might be a nice-to-have. Maybe Assuming that each plugin 'knows' what text format it can process, that pre-processor can easily be handled by Apprise.

Again: thanks and kind regards from Germany! 😄

@caronc caronc merged commit d7166a2 into caronc:master Dec 16, 2023
12 checks passed
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.

4 participants