Skip to content

Conversation

@NeilHanlon
Copy link

Description:

Related issue (if applicable): #

This PR

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • No lint errors (use tox -e lint and even tox -e format to autofix what it can)
  • Test coverage added (use tox -e minimal)

Testing

Added / fixed available tests.

@caronc
Copy link
Owner

caronc commented Sep 21, 2025

I can't accept this PR, you've checked in (what appears to be) your entire development branch (2000+ files) and not just the files you intended to update.

Please review what you've submitted and fix your pull request.

@NeilHanlon
Copy link
Author

Yikes I'm not sure how i managed that. Must sleep more.

Will fix this up, thanks.

@codecov
Copy link

codecov bot commented Sep 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.65%. Comparing base (27091b5) to head (218b7bb).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1418   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         174      174           
  Lines       22554    22554           
  Branches     3587     3587           
=======================================
  Hits        22476    22476           
  Misses         70       70           
  Partials        8        8           
Files with missing lines Coverage Δ
apprise/utils/parse.py 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Owner

@caronc caronc left a comment

Choose a reason for hiding this comment

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

Overall, thank you for updating your PR! It's much cleaner now! 🚀

r"\s*([a-z0-9]{2,3}[0-9][a-z0-9]{3}(?:-[a-z0-9]{1,2})?)"
r"(?=$|[\s,]+[a-z0-9]{4,6})",
r"\s*([a-z0-9]{1,2}[0-9][a-z0-9]{1,3}(?:-(?:[0-9]|1[0-5]))?)"
r"(?=\s*$|\s*[,;\s]+)",
Copy link
Owner

Choose a reason for hiding this comment

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

The "look ahead" should also include the prefix for the next potential match, not just the delimiter to get to it (see how it was before). It's just a subtle change needed here I think. The real issue may have been that the value before was correct, but there were no test cases to back it up? Your current solution may be okay (if provided multiple inputs)?

Do the following work:

  • CSVAR, CSVAR2,,,,
  • CSVAR, CSVAR2-CID, CSVAR3-CID, CSVAR4,,,,
  • CSVAR, CSVAR2
  • CSVAR, INVALIDCS;,,, CSVAR2,,,,

Where CSVAR should be valid variations of callsigns (including the new shortened version you introduced)?

I think if the look-ahead in the prefix doesn't also include the validity of the next item too, you'll generate successfully on blank matches in situations where the input ends with a delimiter?

I want to be clear; i don't know that it's also possible what you have is okay too; I hadn't had a chance to look closer at it.

Thoughts? Does this safely cover this? I didn't see any added tests in tests/test_apprise_utils.py to test parse_call_sign()

Copy link
Author

Choose a reason for hiding this comment

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

The reason I went down this path at all is that my own call: N1HAN, is invalid according to the existing regex. I'll take a closer look at this PR again with fresh eyes and see if anything different stands out. You've raised some good points I didn't consider in my initial pass.

Copy link
Owner

Choose a reason for hiding this comment

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

neil-forks-apprise-callsign.patch

I had a chance to look at your code and fix it. I don't have permission to change your master branch, so here is the solution that should accomodate you and me at the same time. If you want to apply it to this branch and resubmit, we should be good to go.

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.

2 participants