Adopt more idiomatic Go style; fix panic in sedra.FindParshaNum - #35
Merged
Conversation
No functional change; all exported signatures and types are unchanged
(verified by diffing `go doc -all` against the previous commit).
sedra: rename the snake_case parsha tables (sat_short, mon_long_leap_Israel,
...) to camelCase, and rename _d/_u to doubled/undoubled with a comment
explaining the negative-index encoding of doubled readings. Split
getSedraArray into commonSedraArray and leapSedraArray, each a switch on
weekday with an inner switch on year type, and collapse the repeated
`if il { ... } else { ... }` into a schedule() helper. Use strings.Builder
rather than bytes.Buffer in Parsha.Render.
hebcal: drop the redundant else-after-return in getStartAndEnd and replace
the four-way HDate comparison with an XOR on hasStart/hasEnd. Rename the
`min` local, which shadows the builtin. Use the AND-NOT operator (&^=) to
clear mask bits instead of `&= ^`.
event: rewrite HolidayEvent.Render's if/else-if chain as a tagless switch.
omer: use := over `var x = ...`, group the loose consts, rewrite Sefira's
locale chain as a switch, and document the exported OmerEvent and
NewOmerEvent.
candles: pass time.Time by value to formatTime rather than by pointer.
Doc comments on exported identifiers now begin with the identifier name,
per Go convention, so that `go doc` renders them correctly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FindParshaNum already declared an (hdate.HDate, error) result, but panicked when the requested parsha was not found in the year's schedule. That branch is reachable: a parsha that is read as part of a doubled reading cannot be found on its own, so in a year that reads Matot-Masei together (e.g. 5784 in the Diaspora), FindParshaNum(42) and FindParshaNum(43) both crashed the caller. Return an error in that case, as the signature always promised. Existing callers already discard the error, so they now see a zero HDate rather than a panic. Also switch the invalid-number path to fmt.Errorf for consistency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gematriya v1.0.1 -> v1.1.0 greg v1.0.2 -> v1.1.0 Co-Authored-By: Michael J. Radwin <mradwin@gmail.com>
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Three commits: a dependency bump, a no-op style pass, and one real bug fix that the style pass turned up.
Stays on Go 1.18 — every change is compatible with the current
go.mod.chore: update dependenciesgematriya v1.0.1 → v1.1.0, greg v1.0.2 → v1.1.0. Independent of the rest of the branch;
go mod tidyis a no-op on the result.Adopt more idiomatic Go style(no functional change)All exported signatures and types are unchanged; verified by diffing
go doc -allfor every package againstmain. The only API-surface differences are doc comment text and receiver names, neither of which callers can observe.sat_short,mon_long_leap_Israel, …) to camelCase, and_d/_utodoubled/undoubledwith a comment explaining the negative-index encoding of doubled readings.getSedraArraywas a nested if/else-if tree in which every branch returned; it's now split intocommonSedraArrayandleapSedraArray, each aswitchon weekday with an innerswitchon year type, with the repeatedif il { ... } else { ... }collapsed into aschedule()helper.Parsha.Renderusesstrings.Builderinstead ofbytes.Buffer.else-after-returningetStartAndEnd; replaced the four-wayHDatecomparison with an XOR onhasStart/hasEnd; renamed theminlocal that shadowed the builtin; used the AND-NOT operator (&^=) to clear mask bits.HolidayEvent.Render's if/else-if chain is now a taglessswitch.:=overvar x = ..., grouped consts,Sefira's locale chain as aswitch, and doc comments on the exportedOmerEvent/NewOmerEvent.formatTimetakestime.Timeby value rather than by pointer.Doc comments on exported identifiers now begin with the identifier name, per Go convention, so
go docrenders them correctly.sedra: return an error from FindParshaNum instead of panickingFindParshaNumalready declared an(hdate.HDate, error)result, but panicked when the parsha wasn't found. That branch is reachable: a parsha read as part of a doubled reading can't be found on its own, so in a year that reads Matot-Masei together (e.g. 5784 in the Diaspora),FindParshaNum(42)andFindParshaNum(43)both crashed the caller.It now returns an error, as the signature always promised. Added
TestFindParshaNumErrorsto cover it.Note
This is a behavior change, not a pure fix — worth a changelog line. Every in-tree caller already discarded the error with
_, so they now get a zeroHDateinstead of a panic. But anyone downstream relying on the panic (catching it withrecover, or treating "didn't panic" as proof the parsha exists) will now silently get a zero value if they ignore the error.Testing
gofmt,go vet, and the full test suite pass at each of the three commits individually.🤖 Generated with Claude Code