APRS (Automated Packet Reporting System) Ham Radio plugin

joergschultzelutter commented 7 months ago

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

[X] apprise/plugins/NotifyAprs.py
[X] KEYWORDS
- new service plugin for sending APRS messages via Apprise.
[x] README.md
- added as local wiki document and README_aprs.md
[x] packaging/redhat/python-apprise.spec
- add new service into the %global common_description

Checklist

[x] The code change is tested and works locally.
[x] There is no commented out code in this PR (applies to the actual plugin)
[x] No lint errors (use flake8)
[x] 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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 :warning:

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

Additional details and impacted files

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

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

caronc commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

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 / apprise