-
-
Notifications
You must be signed in to change notification settings - Fork 528
fix parsing of USA callsigns--specifically different prefix/suffix counts like 1x2 and 1x3 calls #1418
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
Open
NeilHanlon
wants to merge
1
commit into
caronc:master
Choose a base branch
from
neil-forks:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+38
−8
Open
fix parsing of USA callsigns--specifically different prefix/suffix counts like 1x2 and 1x3 calls #1418
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
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.pyto testparse_call_sign()There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.