-
Notifications
You must be signed in to change notification settings - Fork 17
Issue 235 - Error getting reference to Jonah #236
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
Conversation
Reviewer's GuideAdds regression tests for Jonah-related book range abbreviations and tightens the regex for matching John so that Jonah (and similar) abbreviations are parsed correctly without being misclassified as John, plus an updated dependency lockfile. Sequence diagram for parsing Jonah-related book range abbreviationssequenceDiagram
actor User
participant Parser
participant BookPatternMatcher
participant JohnRegex
participant JonahRegex
User->>Parser: parse_reference_string("micah-jon")
Parser->>BookPatternMatcher: tokenize_and_match_books("micah-jon")
BookPatternMatcher->>JonahRegex: test("jon")
JonahRegex-->>BookPatternMatcher: match(Jonah)
BookPatternMatcher->>JohnRegex: test("jon")
JohnRegex-->>BookPatternMatcher: no_match
BookPatternMatcher-->>Parser: [Micah, Jonah]
Parser-->>User: ParsedRange(Micah, Jonah)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
…e-to-jonah' into issue/235-error-getting-reference-to-jonah
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
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.
Hey - I've left some high level feedback:
- The
_JOHN_REGULAR_EXPRESSIONnegative lookahead is becoming quite dense (e.g.,shua|b|nah|el|n); consider adding an inline comment or extracting the excluded suffixes into a named helper to make the intent and future maintenance clearer. - Given that the Jonah collision arises from
Jo-style abbreviations, you might want to review whether adding word boundaries or more explicit tokenization around book abbreviations would be a more robust long-term approach than continuing to extend the negative lookahead list.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_JOHN_REGULAR_EXPRESSION` negative lookahead is becoming quite dense (e.g., `shua|b|nah|el|n`); consider adding an inline comment or extracting the excluded suffixes into a named helper to make the intent and future maintenance clearer.
- Given that the Jonah collision arises from `Jo`-style abbreviations, you might want to review whether adding word boundaries or more explicit tokenization around book abbreviations would be a more robust long-term approach than continuing to extend the negative lookahead list.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Pull request overview
This PR fixes issue #235 where the abbreviation "Jon" for the book of Jonah was incorrectly being matched by the JOHN regex pattern, causing parsing errors when used in book ranges like "micah-jon" or "jon-jon".
Changes:
- Updated the JOHN regular expression to exclude matching "Jo" when followed by "n", preventing conflicts with the Jonah abbreviation "Jon"
- Added comprehensive parameterized tests covering all reported cases where "Jon" abbreviation was causing issues
- Bumped package version from 0.15.3 to 0.15.4
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pythonbible/books.py | Modified _JOHN_REGULAR_EXPRESSION to add |n to the negative lookahead, preventing "Jon" from matching JOHN |
| tests/parser/parser_test.py | Added parameterized test test_issue_235_jonah_abbreviation covering all four reported edge cases with Jonah abbreviation |
| uv.lock | Updated package version to 0.15.4 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
This PR resolves #235
Checklist:
Summary by Sourcery
Tests:
Summary by Sourcery
Resolve incorrect parsing of Jonah and John book abbreviations and add regression tests for Jonah-related reference ranges.
Bug Fixes:
Tests: