Parse dotted abbreviations, multi-word books, and unicode dashes - #366
Conversation
|
Btw the first 2 screenshots are after my fixes and the 3rd one is the error I get before the fix. Now there is no error. |
|
Nice direction — the looser prose matching makes the suggester feel a lot more natural, and the dash normalisation is a good catch. I ran the branch locally (all 380 tests pass after 1. Period stripping is one-sided (
|
| call | master | this branch |
|---|---|---|
bookIdFromTranslationAndName('de', '1. Mose') |
1 |
throws No book matched "1. Mose" |
bookIdFromTranslationAndName('de', '5. Mose') |
5 |
throws |
bookIdFromTranslationAndName('ro', 'F. Ap.') |
44 |
throws |
bookIdFromName('1. Mose') |
1 |
throws |
Since this is public API of the published bible-reference-toolkit, normalising both sides (strip periods / collapse whitespace on each exactNames entry too) would probably be the safer shape.
2. The new try/catch may be guarding one call too early (getSuggestionsFromQuery.ts:43)
splitBibleReference returns the candidate unchanged when no book resolves (resolveBookName falls through), so parsing succeeds and execution reaches localizedBookName → getFullBookName, which is where the throw actually happens.
getSuggestionsFromQuery('Notabook 1:1', DEFAULT_SETTINGS) // rejects: No book matched "Notabook"
getSuggestionsFromQuery('todo item 3: 1 thing', DEFAULT_SETTINGS) // same
The second one matters a bit more — verseMatch and hasExplicitVerse both accept it, so the editor suggester really does call this on that keystroke. Pulling localizedBookName inside the same try and returning [] would match the hunk's stated intent.
3. Prose trimming doesn't reach numbered books (splitBibleReference.ts:69, with regs.ts:57)
BOOK_REG's leading ([123])* can't span the intervening prose, so the match stops short of the ordinal and resolveBookName never sees the book. The unfortunate part is that it fails silently rather than loudly — so the wrong verse gets inserted:
splitBibleReference('see 1 Cor. 13:4') // { bookName: 'see', chapterNumber: 1, verseNumber: 4 }
splitBibleReference('read 1 John 1:1') // { bookName: 'read', chapterNumber: 1, verseNumber: 1 }
(safeParseInt('1 Cor. 13') returns 1, since parseInt happily takes the numeric prefix.) The covered case 'see Eph. 2:8' works fine, which is what hides it. Either extending the trimming to the numbered case or making safeParseInt reject trailing garbage would help.
4. Off-by-two in the replacement start (VerseEditorSuggester.ts:106)
queryContent = currentContent.substring(2), so queryContent.lastIndexOf(verseMatchResult) is 2 less than the real line column. This was harmless while the match always began at the book name, but the widened BOOK_NAME means it can now start up to 5 words earlier — even on a leading space:
--i am reading Genesis 1:1
verseMatch returns " am reading Genesis 1:1" at index 1, so start.ch = 1 where the true column is 3, and accepting the suggestion leaves a stray - in front of the inserted verse. A + 2 should do it.
A few things I checked that look fine: the {0,5} repetition in BOOK_NAME doesn't backtrack badly (worst case I found was 3.1 ms on a 300-char input); no book name or abbreviation in any of the 14 catalogs or base.json contains a DASH_CHARS character, so the normalizeDashes safety comment holds; the removed regex exports have no remaining importers; and isIncompleteReference behaves for John 3:16-a, John 3:16-4:2, John 3:a and each dash variant.
Thanks for putting this together — #1 and #3 are the two I'd most want resolved, since both change what verse a reader ends up with.
|
Issue 4 was real but 3 was not. 1 and 2 were more complex to fix because they were not direct issues. |
|
Thanks for turning these around so quickly — I pulled the branch again and re-ran everything, and 1, 2 and 4 all check out: 438 tests pass. On 3 — you're right that it isn't reachable any more, so no argument about the outcome. Just noting for the record that the parse itself is unchanged, in case it matters later: The new One genuine question rather than a request. The gate also turns off prose in front of a reference entirely: Since that was part of what the PR set out to add, I wanted to check whether dropping it is the intended landing spot or just fell out of the fix. Both are fine by me — strict is easier to reason about, and "the reference has to be the whole query" is a rule users can actually predict. If it is intentional, the PR description is worth a small tweak so the behaviour isn't a surprise later. Only other thing left is the docstring on Otherwise this looks good to me. Thanks again for the care on the tests, the added coverage is the best part of the diff. |
|
What? Hey @tim-hub (the REAL Tim). AI is cool and all but this is too much in my opinion. The word salad the AI put out was hard to follow since it changes it's mind from sentence to sentence. The code is perfectly fine and works from testing. |
OK I will have a look again |
|
All good, merge in now and will have a fllowing up change for formatting then I will cut a release for this. |
Periods. Publishers and writers usually add citations with periods (like
Eph. 2:8). The code required the book's letters to be followed directly by the chapter digits, soEph. 2:8matched nothing and the suggester never fired.Unicode dashes. It's also not uncommon to see the use of different dashes used in verse ranges. Regular dashes are almost unseen in published book from what I have read. So
John 3:16–18(en dash) matched asJohn 3:16, fell through to the single-verse path, andparseInt("16–18")returned16— no error was thrown.Multi-word book names.
BOOK_REGstopped at the first space, soSong of Solomon 1:1extracted bookSongand threw.Errors while still typing. Typing
John 3:16-18necessarily passes throughJohn 3:16-, which was parsed and logged asInvalid verse range "16-".