refactor: simplify calculation of days and other part of IPeriod #57
Conversation
Seddryck
changed the title
|
WalkthroughMultiple files have been updated to adjust how date period data is defined and processed. The YAML definitions for various date structures no longer include a duration or an "intrayear" attribute; instead, a "byYear" attribute is used. In parallel, the source generator logic has been modified to output the new properties and simplified date calculations. Additionally, a new test case has been added to the YearMonth test suite to verify the correct day count for October 1582. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant YM as YearMonthTests
participant YMCalc as YearMonth / DateCalc Module
TR->>YM: Run Days_GregorianCalendar_Expected(input, expected)
YM->>YMCalc: Calculate Days in October 1582
YMCalc-->>YM: Return computed day count
YM-->>TR: Test result (pass/fail)
sequenceDiagram
participant SD as StructDefinition
participant SG as StructGenerator
participant TP as Templates/Period.scriban-cs
participant GC as Generated Code
SD->>SG: Request GeneratePeriod(structDefinition)
SG->>SD: Retrieve period attributes (ByYear, Year, YearDuration)
SG->>TP: Render period template with new properties (by_year, year value/duration)
TP-->>SG: Return generated period code
SG-->>GC: Output final code structure
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/_data/structs.yml(3 hunks)src/Chrononuensis.Core.Testing/YearMonthTests.cs(1 hunks)src/Chrononuensis.Core/Structs.yml(3 hunks)src/Chrononuensis.SourceGenerator/Definitions/StructPeriod.cs(1 hunks)src/Chrononuensis.SourceGenerator/StructGenerator.cs(1 hunks)src/Chrononuensis.SourceGenerator/Templates/Period.scriban-cs(1 hunks)
🔇 Additional comments (11)
src/Chrononuensis.SourceGenerator/Definitions/StructPeriod.cs (1)
12-12: LGTM! Property changes improve clarity.The introduction of
ByYearproperty and removal ofIntrayearandDurationproperties simplifies the period definition while making the intent clearer.src/Chrononuensis.SourceGenerator/Templates/Period.scriban-cs (3)
7-8: LGTM! Simplified days calculation.The calculation has been simplified to a more direct and readable formula.
13-14: LGTM! Improved date calculation logic.The new
by_yearbased calculation provides a clearer way to handle month divisions within a year.
22-23: LGTM! Consistent date range handling.The
LastDatecalculation now consistently uses theby_yearvalue to determine period boundaries.src/Chrononuensis.SourceGenerator/StructGenerator.cs (1)
231-238: LGTM! Better organization of year-related data.The changes improve the structure by:
- Using
by_yearfor period divisions- Grouping year-related properties into a single object
docs/_data/structs.yml (3)
49-50: Define YearMonth Subdivision withbyYear
The addition ofbyYear: 12in the YearMonth structure clearly specifies that a year is divided into 12 parts, aligning with the new design approach. This makes the period definition more explicit and consistent with related structures.
61-62: Standardize YearQuarter Definition
IntroducingbyYear: 4for the YearQuarter structure appropriately defines that there are 4 quarters per year. This change mirrors the updated logic in the core files and enhances clarity.
73-74: Clarify YearSemester Partitioning
Replacing the previous intrayear approach withbyYear: 2for YearSemester clearly indicates the year is split into 2 semesters. This improves consistency and maintainability across all period-based structures.src/Chrononuensis.Core/Structs.yml (3)
49-50: Update YearMonth Period Definition
The newbyYear: 12attribute in the YearMonth structure succinctly establishes that the year is divided into 12 parts, which is in line with the overall strategy to simplify date period representations.
61-62: Consistent Quarter Segmentation
WithbyYear: 4added to the YearQuarter structure, the specification now explicitly states that there are 4 quarters in a year, thereby aligning with traditional calendar division and similar updates in the documentation.
73-74: Standardize Semester Configuration
The introduction ofbyYear: 2for the YearSemester structure standardizes the approach for defining period subdivisions. This change enhances both clarity and consistency across the system's various date structures.
| [Test] | ||
| [TestCase("1582-10", 31)] | ||
| public void Days_GregorianCalendar_Expected(string input, int expected) | ||
| { | ||
| var value = YearMonth.Parse(input, null); | ||
| Assert.That(value.Days, Is.EqualTo(expected)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify historical accuracy of the October 1582 test case.
The test case for October 1582 assumes 31 days, but historically, this month had only 21 days in the Gregorian calendar as days 4-14 were skipped during the calendar reform.
🌐 Web query:
How many days were there in October 1582 when the Gregorian calendar was introduced?
💡 Result:
The introduction of the Gregorian calendar in October 1582 resulted in 21 calendar days for that month in regions adopting the new system[1][2][3][7][8][9]. Here’s why:
Calendar Transition Details
- The Julian calendar (used since Julius Caesar’s time) had accumulated a 10-day discrepancy with astronomical events like the spring equinox due to its overestimation of solar year length[1][2][7].
- To correct this:
- October 4, 1582 (Julian) was designated as the last day of the old system.
- The next day became October 15, 1582 (Gregorian), eliminating dates October 5–14[1][3][7].
- This adjustment aligned seasonal markers (e.g., equinoxes) with liturgical requirements for Easter[1][2].
Regional Adoption
- Catholic nations like Italy, Spain, Portugal, and Poland adopted this change immediately[3][8].
- Protestant and Orthodox countries resisted initially:
- England switched in 1752 (skipping 11 days)[9].
- Russia delayed until 1918[8].
Key Implications
- The month retained its usual 31-day structure but skipped 10 consecutive dates[7].
- Days of the week continued uninterrupted: Thursday (October 4) was followed by Friday (October 15)[3][7].
This reform standardized timekeeping globally over centuries while resolving discrepancies critical to religious observances[1][5].
Citations:
- 1: https://www.walksinrome.com/blog/october-4th-1582-last-day-of-the-julian-calendar
- 2: https://encyclopediavirginia.org/entries/the-gregorian-calendar/
- 3: https://en.wikipedia.org/wiki/1582
- 4: https://www.legis.state.pa.us/cfdocs/legis/SpeakerBios/CalendarChanges.cfm
- 5: https://www.britannica.com/story/ten-days-that-vanished-the-switch-to-the-gregorian-calendar
- 6: https://www.youtube.com/watch?v=63ap3QE2zEs
- 7: https://secwww.jhuapl.edu/techdigest/content/techdigest/pdf/V03-N04/03-04-Newton.pdf
- 8: https://www.upworthy.com/why-the-calendar-skipped-from-october-4th-to-the-15th-in-1582
- 9: https://www.family-historian.co.uk/help/fh7/thegregoriancalendar.html
- 10: https://www.nottingham.ac.uk/manuscriptsandspecialcollections/researchguidance/datingdocuments/juliangregorian.aspx
Update the expected days count for October 1582.
The test case in src/Chrononuensis.Core.Testing/YearMonthTests.cs currently asserts that October 1582 has 31 days. However, due to the Gregorian calendar reform—which skipped October 5–14—October 1582 actually contained only 21 calendar days. Please update the expected value in the test case accordingly.
There was a problem hiding this comment.
No because it's not proletic Georgian CAlendar
There was a problem hiding this comment.
@Seddryck Thank you for the correction. You're absolutely right - since we're using the proleptic Gregorian calendar, October 1582 should indeed have 31 days as the test correctly asserts.
✏️ Learnings added
Learnt from: Seddryck
PR: Seddryck/Chrononuensis#57
File: src/Chrononuensis.Core.Testing/YearMonthTests.cs:178-184
Timestamp: 2025-02-20T20:39:00.092Z
Learning: The codebase uses proleptic Gregorian calendar which extends Gregorian calendar rules backwards in time, meaning dates before the historical Gregorian calendar adoption (October 1582) are calculated as if Gregorian calendar was always in effect. For example, October 1582 has 31 days in proleptic calendar (unlike 21 days in historical Gregorian calendar).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Refactor
Tests