Skip to content

refactor(bible): centralize book-name maps in bible.ts#3

Open
euxaristia wants to merge 1 commit intomasterfrom
refactor/centralize-book-maps
Open

refactor(bible): centralize book-name maps in bible.ts#3
euxaristia wants to merge 1 commit intomasterfrom
refactor/centralize-book-maps

Conversation

@euxaristia
Copy link
Copy Markdown
Owner

Summary

  • Exports LATIN_BOOK_NAMES, SPANISH_BOOK_NAMES, ABBREVIATION_MAP, DEFAULT_LATIN_VERSION, DEFAULT_SPANISH_VERSION from bible.ts and imports them in message-handler.ts, eliminating ~370 lines of duplicated constants.
  • Addresses Gemini's follow-up comment on fix(bible): add Latin accusative alias "psalmos" for Psalms #2 about shared-map synchronization risk.
  • Fixes two drift bugs found during consolidation:
    • "tobias" was self-mapped in message-handler (should be "tobit").
    • "rut" (Spanish) was missing from bible.ts's map.

Test plan

  • deno test -A tests/bible.test.ts tests/message-handler.test.ts — 130/130 passing

Export LATIN_BOOK_NAMES, SPANISH_BOOK_NAMES, ABBREVIATION_MAP, and
default-version constants from bible.ts and import them in
message-handler.ts. Removes duplicated maps that had already drifted
("tobias" previously mapped to itself instead of "tobit", "rut" was
missing from bible.ts).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the codebase by exporting constants from src/services/bible.ts for use in src/services/message-handler.ts and adds support for the book of Ruth in the abbreviation map. The review identified a critical bug where Spanish references incorrectly default to the Latin Vulgate version, as well as a need to consolidate duplicate normalizeBookName logic. Additionally, the reviewer noted missing Spanish book names in the detection set and missing abbreviations for 'amós' and 'baruc' in the mapping.

BibleService,
BibleVerse,
DEFAULT_LATIN_VERSION,
DEFAULT_SPANISH_VERSION,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

In the processMessage function (line 691), fallbackVersion is incorrectly set to DEFAULT_LATIN_VERSION when a Spanish book name is detected. This causes all Spanish references (e.g., "Juan 3:16") to return Latin text from the Vulgate instead of the Spanish translation. It should be changed to DEFAULT_SPANISH_VERSION.

Additionally, the normalizeBookName function in this file (line 35) is now a duplicate of the one in bible.ts and contains a bug in its candidate sorting logic (it doesn't correctly find the minimum length across all abbreviations). Consider exporting the version from bible.ts and using it here to ensure consistency and fix the bug.

Comment thread src/services/bible.ts
Comment on lines +213 to 216
export const SPANISH_BOOK_NAMES = new Set([
"génesis", "éxodo", "exodo", "levítico", "levitico", "números", "numeros",
"deuteronomio", "josué", "josue", "jueces", "1 reyes", "2 reyes",
"1 crónicas", "1 cronicas", "2 crónicas", "2 cronicas", "esdras",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The SPANISH_BOOK_NAMES set is missing several books that should trigger Spanish auto-detection, specifically "rut" and "ester". While "rut" was added to the ABBREVIATION_MAP in this PR, it was missed here. Including these ensures that references like "Rut 1:1" or "Ester 2:5" are correctly identified as Spanish and use the appropriate default version.

Suggested change
export const SPANISH_BOOK_NAMES = new Set([
"génesis", "éxodo", "exodo", "levítico", "levitico", "números", "numeros",
"deuteronomio", "josué", "josue", "jueces", "1 reyes", "2 reyes",
"1 crónicas", "1 cronicas", "2 crónicas", "2 cronicas", "esdras",
export const SPANISH_BOOK_NAMES = new Set([
"génesis", "éxodo", "exodo", "levítico", "levitico", "números", "numeros",
"deuteronomio", "rut", "ester", "josué", "josue", "jueces", "1 reyes", "2 reyes",
"1 crónicas", "1 cronicas", "2 crónicas", "2 cronicas", "esdras",

Comment thread src/services/bible.ts
"josué": "joshua",
"josue": "joshua",
"jueces": "judges",
"rut": "ruth",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Missing Spanish abbreviations for "amós" and "baruc" in the ABBREVIATION_MAP. Adding these improves the robustness of the reference parser for Spanish users.

Suggested change
"rut": "ruth",
"rut": "ruth",
"amós": "amos",
"baruc": "baruch",

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.

1 participant