Skip to content

Restore northern court era preference on jd lookup - #24

Merged
sugi merged 2 commits into
masterfrom
fix/north-court-era-priority
Jul 13, 2026
Merged

sugi merged 2 commits into
masterfrom
fix/north-court-era-priority

Conversation

@sugi

@sugi sugi commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Summary

Reverts the behavioral part of #23's southern-court era lookup (6891fda). The README statement that conversion from Date/jd should yield southern-court eras during the Nanboku-cho period was wrong to begin with — northern-court eras are the conventional choice — so this restores the original behavior and fixes the README instead.

  • Utils.find_era goes back to the original reverse_each scan over ERA_DEFS: northern eras win on overlapping ranges (1332 → 正慶, 1340 → 暦応, 1350 → 観応, 1391 → 明徳). Periods with no northern-specific era (e.g. 元弘 around 正慶, 延元 before the 暦応 era change) still resolve to the southern era, as before Strict date validation, southern-court era lookup, and comprehensive review fixes #23.
  • The ERA_JD_LOOKUP / NORTH_COURT_ERA_NAMES constants (and their spec) are removed; the bsearch optimization introduced together with the southern preference is reverted along with it.
  • Both courts' era names remain parseable (specs added for southern-court parsing: 正平, 元中).
  • README now documents the actual behavior; the unreleased 2.0.0 ChangeLog entry is corrected to note the behavior is unchanged from 1.1.0.

Test plan

  • TDD: spec expectations flipped to northern eras first and confirmed failing against the southern-preference code, then the revert made them pass
  • bundle exec rspec: 65 examples, 0 failures; RuboCop: no offenses
  • End-to-end smoke check of Date#strftime('%Je%Jg年...') across the schism period, including southern-era parse round-trips

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q1Pf4Mrtd7zB8Lrjc499t9

Revert the behavioral part of 6891fda: the README statement that
conversion should yield southern court eras was wrong; northern court
eras are the conventional choice. find_era goes back to the original
reverse_each scan over ERA_DEFS (northern eras win on overlapping
ranges), and the ERA_JD_LOOKUP / NORTH_COURT_ERA_NAMES constants are
removed. README and ChangeLog now describe the actual behavior.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q1Pf4Mrtd7zB8Lrjc499t9

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

Copy link
Copy Markdown

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 changes the era lookup behavior during the Nanboku-cho period to prefer northern court eras instead of southern court eras, updating the documentation, tests, and removing the ERA_JD_LOOKUP cache in favor of a reverse scan over ERA_DEFS. A review comment suggests adding a guard clause in find_era to quickly return nil for dates prior to the first defined era, avoiding an unnecessary full-array scan.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread lib/wareki/utils.rb
Comment on lines 114 to 122
def find_era(d)
jd = _to_jd(d)
era = ERA_JD_LOOKUP.bsearch { |e| e.end >= jd }
era && era.start <= jd ? era : nil
ERA_DEFS.reverse_each do |e|
e.start > jd and next
e.end < jd and next
return e
end
nil
end

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

For dates prior to the first defined era (645 AD, start of the 大化 era), or when jd is 0 (e.g., if d is nil and coerces to 0), the loop will scan the entire ERA_DEFS array of 255 elements before returning nil.

Adding a guard clause like jd < ERA_DEFS.first.start and return nil avoids this unnecessary full-array scan and makes the behavior consistent with find_year.

    def find_era(d)
      jd = _to_jd(d)
      jd < ERA_DEFS.first.start and return nil
      ERA_DEFS.reverse_each do |e|
        e.start > jd and next
        e.end < jd and next
        return e
      end
      nil
    end

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

[ClaudeCode] The observation is correct, but the only caller of find_era is Wareki::Date.jd, which raises UnsupportedDateRange immediately when nil is returned — so the guard would only speed up that error path by one scan over ~250 structs (sub-microsecond, once per failed conversion). Since this PR is a faithful revert to the 1.1.0 implementation, we're leaving the extra guard out here rather than mixing in new tweaks.

Copilot AI left a comment

Copy link
Copy Markdown

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 restores the original Nanboku-chō era-resolution behavior when converting from Date/Julian day by reverting the southern-court preference introduced in #23, and updates documentation/changelog to match the (restored) behavior.

Changes:

  • Revert Utils.find_era from ERA_JD_LOOKUP+bsearch back to a ERA_DEFS.reverse_each scan so northern-court eras win in overlapping ranges.
  • Remove ERA_JD_LOOKUP / NORTH_COURT_ERA_NAMES and the corresponding spec.
  • Update README and ChangeLog to document northern-court preference; add parse specs ensuring southern-court era names remain accepted.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
spec/utils_spec.rb Updates expectations for northern-court preference on JD lookup; adds southern-court parse coverage; removes lookup-constant invariant spec.
README.md Corrects documentation to state JD/Date conversion prefers northern-court eras during Nanboku-chō (with noted exceptions).
lib/wareki/utils.rb Reverts era lookup implementation to ERA_DEFS.reverse_each to restore prior overlap resolution behavior.
lib/wareki/common.rb Removes constants introduced for the southern-preference bsearch lookup table.
ChangeLog Updates unreleased entry to reflect documentation correction and unchanged behavior vs 1.1.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/wareki/utils.rb
Comment on lines +116 to +121
ERA_DEFS.reverse_each do |e|
e.start > jd and next
e.end < jd and next
return e
end
nil

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

[ClaudeCode] The concern is valid, but two corrections: (1) this isn't a regression against released behavior — 1.1.0 returned these same mutable structs; the frozen ERA_JD_LOOKUP only existed briefly on master and is what this PR reverts. (2) A frozen copy per call would add an allocation on the conversion hot path and still leave the same mutable structs exposed via ERA_BY_NAME. We've addressed the root cause instead by freezing the Era structs at definition time, covering both lookup paths with no per-call cost: #27

@sugi
sugi merged commit 96ae974 into master Jul 13, 2026
9 checks passed
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