PGP dates #125
PGP dates #125
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the """ WalkthroughThis update introduces a new Seleucid calendar converter, exposes it in the public API, and adds a corresponding calendar type. The Hebrew and Islamic date grammars are enhanced to support optional weekday prefixes and variant month spellings. The Undate and Date classes gain new methods and properties, with expanded and refined tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SeleucidDateConverter
participant HebrewDateConverter
User->>SeleucidDateConverter: to_gregorian(year, month, day)
SeleucidDateConverter->>SeleucidDateConverter: Adjust year with SELEUCID_OFFSET
SeleucidDateConverter->>HebrewDateConverter: to_gregorian(adjusted_year, month, day)
HebrewDateConverter-->>SeleucidDateConverter: (gregorian_year, month, day)
SeleucidDateConverter-->>User: (gregorian_year, month, day)
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (2)
src/undate/converters/calendars/hebrew/transformer.py (1)
6-9: Consider deprecating the HebrewUndate classThe HebrewUndate class appears to be unused now that you're returning a generic Undate with a calendar parameter. If it's no longer needed, consider deprecating or removing it to avoid confusion.
-class HebrewUndate(Undate): - """Undate convience subclass; sets default calendar to Hebrew.""" - - calendar = Calendar.HEBREW -src/undate/converters/calendars/seleucid.py (1)
1-24: Add validation for month and day parametersThe implementation assumes all Seleucid dates use the same month structure as Hebrew dates. Consider adding validation for month and day parameters to ensure they're within valid ranges for the Seleucid calendar.
def to_gregorian(self, year: int, month: int, day: int) -> tuple[int, int, int]: """Convert a Seleucid date, specified by year, month, and day, to the Gregorian equivalent date. Uses hebrew calendar conversion logic with :attr:`SELEUCID_OFFSET`. Returns a tuple of year, month, day. """ + # Validate month and day are within valid ranges for Seleucid calendar + if not 1 <= month <= 13: + raise ValueError(f"Month must be between 1 and 13, got {month}") + # Add further validation as needed return super().to_gregorian(year + self.SELEUCID_OFFSET, month, day)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/undate/converters/calendars/__init__.py(1 hunks)src/undate/converters/calendars/hebrew/hebrew.lark(3 hunks)src/undate/converters/calendars/hebrew/transformer.py(2 hunks)src/undate/converters/calendars/islamic/islamic.lark(4 hunks)src/undate/converters/calendars/seleucid.py(1 hunks)src/undate/date.py(1 hunks)src/undate/undate.py(5 hunks)tests/test_converters/test_calendars/test_hebrew/test_hebrew_transformer.py(1 hunks)tests/test_converters/test_calendars/test_islamic/test_islamic_transformer.py(1 hunks)tests/test_date.py(2 hunks)tests/test_undate.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/test_converters/test_calendars/test_hebrew/test_hebrew_transformer.py (2)
src/undate/converters/calendars/hebrew/transformer.py (1)
HebrewUndate(6-9)src/undate/date.py (1)
DatePrecision(143-168)
src/undate/converters/calendars/__init__.py (1)
src/undate/converters/calendars/seleucid.py (1)
SeleucidDateConverter(5-24)
tests/test_converters/test_calendars/test_islamic/test_islamic_transformer.py (2)
src/undate/converters/calendars/islamic/transformer.py (1)
IslamicUndate(6-9)src/undate/date.py (1)
DatePrecision(143-168)
🔇 Additional comments (25)
tests/test_converters/test_calendars/test_islamic/test_islamic_transformer.py (1)
31-31: Good addition of month-level precision test case.The new test case for "Rabīʿ I 490" properly validates parsing of Islamic dates with month-level precision. This complements your grammar extensions for optional weekday prefixes and improves test coverage.
src/undate/converters/calendars/__init__.py (2)
4-4: Proper import of new Seleucid calendar converter.The import statement correctly brings in the SeleucidDateConverter class from the appropriate module.
6-11: Correctly exposed SeleucidDateConverter in public API.The SeleucidDateConverter is properly added to the
__all__list, making it accessible to users. The organization of the list is maintained with consistent formatting.tests/test_converters/test_calendars/test_hebrew/test_hebrew_transformer.py (1)
29-34: Well-structured test cases for weekday parsing.The test cases thoroughly validate Hebrew date parsing with weekday prefixes, with and without comma separation. The comment about whitespace handling raises a valid question - consider whether ignoring whitespace entirely is the desired behavior, especially for edge cases.
Do you want this behavior where "Thursday12Sivan4795" parses successfully without any whitespace? While flexible parsing can be helpful, it might also allow malformed inputs to be accepted silently.
tests/test_undate.py (2)
301-304: Improved error message pattern in test assertion.The updated regex pattern provides a more precise match for the enhanced error message. This change properly aligns the test with improvements to the error reporting in the
__lt__method.
308-311: Consistent error message pattern for reversed comparison.The same improved pattern is correctly applied to the second test case, maintaining consistency when testing the comparison in the reverse direction.
src/undate/converters/calendars/hebrew/transformer.py (2)
16-16: Good addition of the calendar class attributeAdding this class attribute ensures consistent calendar type usage throughout the transformer.
27-29: Nice comment fix and implementation updateThe comment now correctly refers to the Hebrew calendar instead of what was likely an incorrect reference to the Islamic calendar. Using the configured calendar attribute in the Undate constructor provides better flexibility and consistency with the broader refactoring.
src/undate/date.py (1)
107-126: Well-implemented weekday propertyThe weekday property is implemented correctly, returning the day of week as an integer (Monday=0, Sunday=6) consistent with Python's datetime.date.weekday(). The implementation cleverly accounts for numpy's Thursday-based weeks and includes clear documentation.
One suggestion: consider adding a test for dates without day precision to verify the property returns None.
tests/test_date.py (2)
1-2: Good import additionAdding the datetime import is necessary for comparing the weekday results with Python's built-in implementation.
56-75: Comprehensive weekday testingThe test thoroughly verifies the weekday property against Python's built-in implementation for a full week of dates, which is excellent.
Consider adding two additional test cases:
- Test that weekday returns None for Date objects with year or month precision
- Test dates in different months/years to ensure consistent behavior
def test_weekday_none(): assert Date(2025).weekday is None assert Date(2025, 1).weekday is Nonesrc/undate/converters/calendars/seleucid.py (2)
5-12: Good implementation of SeleucidDateConverterThe class is well-structured with clear documentation for the offset constant and proper attribute overrides.
13-18: Appropriate calendar type overrideThe override of the transformer's calendar attribute to Calendar.SELEUCID ensures proper calendar type identification throughout the system.
src/undate/converters/calendars/islamic/islamic.lark (3)
6-6: Added weekday support to Islamic date grammarThe grammar rule has been updated to include an optional weekday prefix before the day-month-year pattern. This enhancement aligns with similar changes in the Hebrew calendar grammar and improves the parsing capabilities.
33-35: Good addition of weekday and comma rulesThe addition of explicit weekday and comma rules enables parsing dates with weekday prefixes. The weekday implementation properly handles all days of the week with an optional comma.
50-50: Enhanced month pattern to support variant spellingsThe regex pattern for
month_6now includes[dḍ]instead of justd, allowing for variant spellings of "Jumādā" with different diacritics, which improves the robustness of the parser.src/undate/undate.py (5)
32-32: Added Seleucid calendar supportThe
Calendarenum has been extended with theSELEUCIDentry, enabling support for the Seleucid calendar system as mentioned in the PR objectives.
195-197: Improved docstring clarity for set_calendar methodThe docstring now clearly indicates that
set_calendaris intended for initialization use only and directs users to the newas_calendarmethod for changing calendars post-initialization.
208-219: Well-implemented as_calendar methodThe new
as_calendarmethod provides a clean way to create a newUndateinstance with the same date values but a different calendar system without performing calendar conversion. This method is properly documented and maintains the original date values.
339-342: Improved error message for incomparable datesThe error message in the
__lt__method now sorts the dates by precision and includes both in the error message, providing clearer context when dates cannot be compared because one falls within another.
428-428: Corrected comment for year propertyThe comment has been updated to correctly specify that an unknown year string is returned when the year is unset but the precision is year or greater (not month or greater), improving documentation accuracy.
src/undate/converters/calendars/hebrew/hebrew.lark (4)
6-6: Added weekday support to Hebrew date grammarThe grammar rule has been updated to include an optional weekday prefix and optional comma before the year, enhancing parsing capabilities for Hebrew dates with weekday information.
34-36: Good addition of weekday and comma rulesThe addition of explicit weekday and comma rules enables parsing dates with weekday prefixes. The implementation properly handles all days of the week with an optional comma, consistent with the Islamic calendar grammar.
50-51: Enhanced month_8 pattern for variant spellingsThe regex pattern for
month_8now supports multiple variants of Heshvan (Heshvan, Ḥeshvan, Marḥeshvan), improving the robustness of the parser for different textual representations.
54-56: Improved month patterns for variant spellingsThe regex patterns for
month_10andmonth_11have been updated to support variant spellings with different diacritics (Tevet/Teveth and Shevat/Shevaṭ), making the parser more flexible with different textual representations.
| def to_gregorian(self, year: int, month: int, day: int) -> tuple[int, int, int]: | ||
| """Convert a Seleucid date, specified by year, month, and day, | ||
| to the Gregorian equivalent date. Uses hebrew calendar conversion | ||
| logic with :attr:`SELEUCID_OFFSET`. Returns a tuple of year, month, day. | ||
| """ | ||
| return super().to_gregorian(year + self.SELEUCID_OFFSET, month, day) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clean implementation of to_gregorian method
The method correctly applies the offset and delegates to the parent implementation, with clear documentation.
Consider adding unit tests for this new converter to verify the offset application is working correctly:
🏁 Script executed:
#!/bin/bash
# Search for existing tests of calendar converters to understand testing pattern
rg -A 5 "test.*converter" --type pyLength of output: 4523
Add unit tests for the Seleucid calendar converter
While the implementation of to_gregorian correctly applies SELEUCID_OFFSET, we need dedicated tests to ensure the offset is always handled as expected. Please add a new test module following the pattern in tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py:
• Create tests/test_converters/test_calendars/test_seleucid/test_seleucid_converter.py
• Write tests for known date pairs, for example:
def test_to_gregorian_offset_applied():
converter = SeleucidConverter()
# e.g., Seleucid year X maps to Gregorian year X + SELEUCID_OFFSET
assert converter.to_gregorian(1, 1, 1) == (1 + converter.SELEUCID_OFFSET, 1, 1)
# add a few additional sample dates• Verify edge cases (start/end of year, leap months/days if applicable)
This will ensure future changes to the offset logic are caught.
🤖 Prompt for AI Agents
In src/undate/converters/calendars/seleucid.py around lines 19 to 24, the
to_gregorian method is implemented correctly but lacks dedicated unit tests.
Create a new test file at
tests/test_converters/test_calendars/test_seleucid/test_seleucid_converter.py
following the pattern of existing calendar converter tests, such as the Hebrew
converter tests. Add test cases that instantiate the SeleucidConverter and
verify that to_gregorian correctly applies the SELEUCID_OFFSET for various known
date inputs, including edge cases like start/end of year and any leap months or
days if relevant.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/unit_tests.yml (1)
56-58: Usewith:input for the Codecov token
The v4 Codecov Action supports atokeninput underwith:. Switching fromenvtowith:makes the intent explicit and aligns with the action’s documentation.- env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + with: + token: ${{ secrets.CODECOV_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/unit_tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/unit_tests.yml
55-55: could not parse as YAML: yaml: line 55: mapping values are not allowed in this context
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/unit_tests.yml
[error] 55-55: syntax error: mapping values are not allowed here
(syntax)
rlskoeser
force-pushed
the
feature/pgp-dates
branch
from
c1dfef9 to
44a3370
Compare
rlskoeser
force-pushed
the
feature/pgp-dates
branch
from
fa23d5f to
8b3030b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #125 +/- ##
===========================================
+ Coverage 96.50% 98.74% +2.23%
===========================================
Files 20 37 +17
Lines 658 1588 +930
===========================================
+ Hits 635 1568 +933
+ Misses 23 20 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/test_converters/test_calendars/test_seleucid.py (1)
59-110: 🛠️ Refactor suggestionClean up extensive commented-out code.
There's a significant amount of commented-out test code that appears to cover important functionality like leap year handling, edge cases, and comparison with Hebrew calendar conversion. This commented code should either be:
- Implemented as proper tests if the functionality is needed
- Removed entirely if it's obsolete or moved elsewhere
The commented code suggests more comprehensive test coverage was planned, including:
- Leap year and leap day handling
- Comparison with Hebrew calendar offsets
- Additional date conversion edge cases
Consider this approach to address the commented code:
-# TODO: update validation error to say seleucid instead of hebrew - -# seleucid_year = 1458 -# converted_date = convert_seleucid_date(f"Tishri {seleucid_year}") -# ... [extensive commented code] ... + def test_leap_year_handling(self): + """Test leap year and leap day conversion.""" + # Implement the leap year tests from commented code + pass + + def test_offset_consistency_with_hebrew(self): + """Test that Seleucid offset calculations are consistent with Hebrew calendar.""" + # Implement the Hebrew calendar comparison tests + passThis will improve test coverage and code maintainability by either implementing the planned tests or removing unnecessary comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/unit_tests.yml(1 hunks)tests/test_converters/test_calendars/test_seleucid.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests.yml
🔇 Additional comments (3)
tests/test_converters/test_calendars/test_seleucid.py (3)
1-4: LGTM: Clean and appropriate imports.The imports are well-organized and include all necessary components for testing the SeleucidDateConverter functionality.
6-31: LGTM: Comprehensive parsing tests with good coverage.The
test_parsemethod effectively tests parsing at all precision levels (day, month, year) and validates all relevant properties of the resultingUndateobjects. The assertions properly verify calendar type, precision, and label formatting.
32-55: LGTM: Thorough Gregorian conversion validation.The
test_gregorian_earliest_latestmethod properly tests the conversion from Seleucid to Gregorian dates, including both specific date conversions and date ranges for month/year precision. The test cases cover different months (Elul, Adar I, Tishri) and validate the earliest/latest date logic.
| assert date.label == f"{date_str} {SeleucidDateConverter.calendar_name}" | ||
|
|
||
|
|
||
| # TODO: update validation error to say seleucid instead of hebrew |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the TODO: Fix validation error messaging.
The TODO comment indicates that validation errors may incorrectly reference "Hebrew" instead of "Seleucid" calendar. This should be investigated and fixed to ensure proper error messaging.
Run this script to check for validation error messages that mention Hebrew when they should mention Seleucid:
Do you want me to help investigate and fix the validation error messaging?
🏁 Script executed:
#!/bin/bash
# Description: Search for validation error messages in SeleucidDateConverter that might incorrectly reference Hebrew calendar
# Search for error messages in the Seleucid converter
ast-grep --pattern 'class SeleucidDateConverter {
$$$
}'
# Also search for error messages that mention Hebrew in the context of Seleucid
rg -A 3 -B 3 "Hebrew" src/undate/converters/calendars/seleucid.pyLength of output: 377
🏁 Script executed:
#!/bin/bash
# Locate the HebrewDateConverter class and its validation/error logic
rg -n "class HebrewDateConverter" -A20 src/undate/converters/calendars/hebrew.py
rg -n "def validate" -A5 -B2 src/undate/converters/calendars/hebrew.py
rg -n "raise ValueError" -A3 -B3 src/undate/converters/calendars/hebrew.pyLength of output: 478
#!/bin/bash
set -e
# 1. Find any converter file with 'hebrew' in its name
echo "=== Files matching *hebrew*.py ==="
find src/undate/converters/calendars -type f -iname "*hebrew*.py" || echo "No files found."
# 2. Locate the class HebrewDateConverter to confirm its file path
echo
echo "=== Locating class HebrewDateConverter ==="
rg -n "class HebrewDateConverter" -A5 -B2 src/undate/converters/calendars/*.py || echo "Definition not found in calendars folder."
# 3. Inspect ValueError raises and literal 'Hebrew' in that file
echo
echo "=== Inspecting error messages in the Hebrew converter ==="
rg -n "raise ValueError" -A3 -B3 src/undate/converters/calendars/*hebrew*.py || echo "No ValueError calls found."
rg -n "\"Hebrew\"" -A1 -B1 src/undate/converters/calendars/*hebrew*.py || echo "No 'Hebrew' literals found."#!/usr/bin/env bash
set -e
echo "=== 1. Locate HebrewDateConverter definition across the repo ==="
rg -n "class HebrewDateConverter" --glob '*.py' || echo "No class definition found."
echo
echo "=== 2. List all files mentioning HebrewDateConverter ==="
rg -l "HebrewDateConverter" --glob '*.py' || echo "No references found."
echo
echo "=== 3. Inspect the converter file for error messages referencing 'Hebrew' or raising ValueError ==="
file=$(rg -l "class HebrewDateConverter" --glob '*.py' | head -n1)
if [ -n "$file" ]; then
echo "Inspecting $file"
rg -n "\"Hebrew\"" -A3 -B3 "$file" || echo "No 'Hebrew' literals in $file."
rg -n "raise ValueError" -A3 -B3 "$file" || echo "No ValueError raises in $file."
else
echo "Cannot inspect: HebrewDateConverter file not found."
fi#!/usr/bin/env bash
set -e
echo "=== 1. List all DateConverter classes across the repo ==="
rg -n "class .*DateConverter" -A2 -B0 --glob '*.py' || echo "No DateConverter classes found."
echo
echo "=== 2. Locate HebrewDateConverter definition ==="
hebrew_file=$(rg -l "class HebrewDateConverter" --glob '*.py' | head -n1 || true)
if [ -n "$hebrew_file" ]; then
echo "Found HebrewDateConverter in: $hebrew_file"
echo
echo "=== 3. Inspecting error messages in $hebrew_file ==="
rg -n "raise ValueError" -A3 -B3 "$hebrew_file" || echo "No ValueError statements."
rg -n "\"Hebrew\"" -A2 -B2 "$hebrew_file" || echo "No 'Hebrew' literals."
else
echo "HebrewDateConverter class not found!"
fiFix SeleucidDateConverter validation messages
Overwrite the inherited Hebrew‐specific error text so that any ValueError in SeleucidDateConverter refers to the “Seleucid calendar” instead of “Hebrew”:
• In src/undate/converters/calendars/seleucid.py:
– Override the relevant validation methods (e.g. validate_year, validate_month, validate_day) and update their ValueError messages to mention “Seleucid calendar.”
• In tests/test_converters/test_calendars/test_seleucid.py (around line 57):
– Update assertions on exception text to expect “Seleucid calendar” in place of “Hebrew calendar.”
Please remove the TODO and ensure both implementation and tests use the correct calendar name.
🤖 Prompt for AI Agents
In tests/test_converters/test_calendars/test_seleucid.py at line 57 and in
src/undate/converters/calendars/seleucid.py, fix the validation error messages
that incorrectly mention "Hebrew" instead of "Seleucid calendar." Override the
validation methods in SeleucidDateConverter to raise ValueErrors with messages
referencing "Seleucid calendar" and update the test assertions to expect these
corrected messages. Remove the TODO comment after making these changes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores