EDTF demo/validation notebook #98
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a new Jupyter notebook Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 review |
✅ Actions performedReview triggered.
|
![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
🧹 Outside diff range and nitpick comments (14)
tests/test_dateformat/edtf/test_edtf_parser.py (2)
16-16: Consider adding more negative year test cases.While the basic negative year cases are covered, consider adding these additional test cases to improve coverage:
- Negative years with month/day: "-1985-04-12"
- Negative years with qualifiers: "-1985?"
- Negative year ranges: "-1985/1985"
Would you like me to help generate these additional test cases?
Also applies to: 18-19
35-40: LGTM! Consider additional unspecified digit patterns.The test cases provide good coverage of Level 2 unspecified digit patterns. To make it even more robust, consider adding:
- Mixed known and unknown digits in month/day: "1984-1X-2X"
- Unspecified digits with qualifiers: "1XXX-12?"
tests/test_dateformat/edtf/test_edtf_transformer.py (2)
18-20: Consider adding more negative year test cases.The new test cases for negative years look good and correctly validate both scientific notation and simple negative year formats.
Consider adding these additional test cases to improve coverage:
# Negative years with month/day components ("-1985-04-12", Undate(-1985, 4, 12)), # Boundary cases ("-0001", Undate(-1)), # Year 1 BCE ("-0000", Undate(0)), # Year 0
37-41: Consider reorganizing test cases by EDTF levels.The new test cases for unspecified digits are comprehensive and correctly validate Level 2 EDTF functionality.
Consider reorganizing the test cases to clearly separate different EDTF levels and features:
# Group test cases by EDTF level level1_testcases = [ # Basic dates ("1984", Undate(1984)), # Scientific notation years ("Y17000002", Undate(17000002)), ("-Y17000002", Undate(-17000002)), # ... other Level 1 cases ] level2_testcases = [ # Unspecified digits ("156X-12-25", Undate("156X", 12, 25)), ("XXXX-12-XX", Undate("XXXX", 12, "XX")), # ... other Level 2 cases ] testcases = level1_testcases + level2_testcasessrc/undate/dateformat/edtf/edtf.lark (2)
42-42: Clean up or implement commented rule.There's a commented-out rule
year_month_unspecified. Should this be implemented or removed?
Line range hint
69-69: Remove or address dangling comment.The comment "negative calendar year?" at the end of the file seems out of place. Since negative years are now implemented, this comment should be removed or updated.
src/undate/dateformat/edtf/transformer.py (1)
69-78: The negative year handling looks good, but could benefit from additional error handling.The implementation correctly handles the negative year prefix, but consider adding validation for the year string format.
Consider adding input validation:
def year_fivedigitsplus(self, items): # strip off the leading Y and convert to integer token = items[0] value = token.value # check if year is negative negative = False if value.startswith("-"): value = value[1:] negative = True + # Validate the year format + if not value.startswith("Y"): + raise ValueError(f"Invalid year format: {token.value}. Must start with 'Y' or '-Y'") + # Remove Y and ensure remaining chars are digits + value = value.lstrip("Y") + if not value.isdigit(): + raise ValueError(f"Invalid year format: {token.value}. Must contain only digits after Y") - year = int(value.lstrip("Y")) + year = int(value) if negative: year = -yeartests/test_dateformat/test_edtf.py (1)
44-45: Consider adding boundary case tests for negative years.The tests for negative years look good, but consider adding tests for boundary cases such as:
- Year -0001 (transition between BCE/CE)
- Minimum supported negative year
# Suggested additional test cases assert EDTFDateFormat().to_string(Undate(-1)) == "-0001" assert EDTFDateFormat().to_string(Undate(-9999)) == "-9999" # if this is your min supported yearsrc/undate/dateformat/edtf/formatter.py (2)
56-63: Consider implementing uncertain/approximate date handling.The TODO comments indicate that uncertain/approximate date handling is not yet implemented. This is an important feature of the EDTF specification that would enhance the library's completeness.
Would you like me to:
- Open an issue to track the implementation of uncertain/approximate date handling?
- Provide example code for implementing this feature according to the EDTF specification?
Line range hint
89-90: Improve handling of empty date case.The comment "how can we have an empty string? probably shouldn't get here" suggests this is an unexpected case. Consider either:
- Throwing an exception with a clear message
- Adding logging to track these occurrences
- Documenting when this might legitimately occur
Here's a suggested improvement:
if parts: return "-".join(parts) - # how can we have an empty string? probably shouldn't get here - return "" + raise ValueError("Unable to format date: no precision components available")src/undate/undate.py (1)
62-62: Consider usingstr.isnumeric().The comment suggests uncertainty about using
str.isnumeric(). This method would be appropriate here as it checks if all characters in the string are numeric characters, which aligns with the validation needs.Consider updating the code to use
str.isnumeric():- try: - year = int(year) - # update initial value since it is used to determine - # whether or not year is known - self.initial_values["year"] = year - min_year = max_year = year - except ValueError: + if isinstance(year, str) and year.isnumeric(): + year = int(year) + # update initial value since it is used to determine + # whether or not year is known + self.initial_values["year"] = year + min_year = max_year = year + else:examples/notebooks/edtf-support.ipynb (3)
8-16: Consider adding a link to the project repository.The introduction clearly explains the notebook's purpose and limitations. Consider adding a link to the undate project repository for readers who want to explore the library further.
729-731: Address the TODO comment for min/max year handling.The comment indicates that min/max year functionality exists in a different branch. This should be addressed to ensure complete EDTF Level 1 support.
Would you like me to help implement the min/max year functionality or create a GitHub issue to track this task?
698-769: Consider documenting performance implications.The unspecified digit functionality involves calculating earliest/latest possible values. Consider adding documentation about:
- Time complexity of these calculations
- Memory usage for large ranges
- Any caching mechanisms in place
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- examples/notebooks/edtf-support.ipynb (1 hunks)
- src/undate/init.py (1 hunks)
- src/undate/dateformat/edtf/edtf.lark (3 hunks)
- src/undate/dateformat/edtf/formatter.py (1 hunks)
- src/undate/dateformat/edtf/transformer.py (1 hunks)
- src/undate/undate.py (2 hunks)
- tests/test_dateformat/edtf/test_edtf_parser.py (2 hunks)
- tests/test_dateformat/edtf/test_edtf_transformer.py (2 hunks)
- tests/test_dateformat/test_edtf.py (2 hunks)
🧰 Additional context used
🪛 Ruff
src/undate/__init__.py
3-3:
undate.undate.Undateimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
undate.undate.UndateIntervalimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
4-4:
undate.date.DatePrecisionimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
🔇 Additional comments (13)
tests/test_dateformat/edtf/test_edtf_parser.py (1)
Line range hint
16-40: Verify test cases against EDTF specification.Let's verify that the test cases align with the EDTF specification documentation.
✅ Verification successful
Let me check the content of the found files to verify the test cases against the specification.
Let me check the actual EDTF parser implementation to verify the test cases.
Let me check the actual parser implementation and test file content.
Let me check the EDTF grammar file to complete the verification.
Based on the comprehensive review of the EDTF implementation, grammar, tests, and example notebook, I can now provide the final response.
Test cases align with EDTF specification levels 0, 1, and 2
The test cases in
test_edtf_parser.pyare well-aligned with the EDTF specification as evidenced by:
Level 0 features:
- Basic dates (e.g., "1984", "1984-05", "1984-12")
- Time intervals (e.g., "1000/2000", "1000-01/2000-05-01")
Level 1 features:
- Letter-prefixed years (e.g., "Y170000002", "-Y170000002")
- Negative years (e.g., "-1985")
- Qualifiers (e.g., "1984?", "2004-06~", "2004-06-11%")
- Unspecified digits (e.g., "201X", "20XX")
- Extended intervals (e.g., "1985-04-12/..", "../1985-04-12")
Level 2 features:
- Partial unspecified (e.g., "156X-12-25", "1XXX-12")
- Masked precision (e.g., "1984-1X")
The implementation is validated by both unit tests and the comprehensive example notebook
edtf-support.ipynbwhich demonstrates parsing and formatting for each EDTF feature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EDTF specification documentation or examples echo "Searching for EDTF specification references..." rg -l "EDTF" --type md rg -l "Extended Date.*Time Format" --type md # Look for any existing EDTF validation or example files echo "Searching for EDTF example files..." fd -e md -e txt -e rst -e ipynb . | rg -i "edtf|date.*format"Length of output: 381
Script:
#!/bin/bash # Check CHANGELOG.md for EDTF specification references echo "=== CHANGELOG.md EDTF references ===" rg -A 5 -B 5 "EDTF|Extended Date.*Time Format" CHANGELOG.md # Check the example notebook echo -e "\n=== EDTF examples from notebook ===" cat examples/notebooks/edtf-support.ipynb | jq -r '.cells[] | select(.cell_type == "markdown" or .cell_type == "code") | .source[]' | grep -A 10 -B 10 "EDTF" # Look for EDTF parser implementation to understand supported features echo -e "\n=== EDTF parser implementation ===" rg -l "class.*EDTFParser"Length of output: 17249
Script:
#!/bin/bash # Search for EDTF parser file echo "=== Finding EDTF parser file ===" fd -e py | rg -i "edtf.*parser" # Look at the parser implementation echo -e "\n=== EDTF parser implementation ===" rg -A 10 "class.*EDTFParser" --type py # Check for EDTF grammar or patterns echo -e "\n=== EDTF patterns ===" rg "EDTF.*pattern" --type py -A 5Length of output: 413
Script:
#!/bin/bash # Check the EDTF parser implementation echo "=== EDTF Parser Implementation ===" cat src/undate/dateformat/edtf/parser.py # Check the test file content echo -e "\n=== EDTF Parser Tests ===" cat tests/test_dateformat/edtf/test_edtf_parser.pyLength of output: 2224
Script:
#!/bin/bash # Check the EDTF grammar file echo "=== EDTF Grammar ===" cat src/undate/dateformat/edtf/edtf.lark # Check if there are any other EDTF-related files echo -e "\n=== Other EDTF files ===" fd . | rg -i "edtf"Length of output: 2624
src/undate/dateformat/edtf/edtf.lark (4)
3-3: LGTM: Added SIGNED_INT import for negative year support.The addition of SIGNED_INT import is appropriate for supporting negative year values in the grammar.
38-40: LGTM: Well-structured unspecified digit support.The implementation of unspecified digits using 'X' follows the EDTF Level 2 specification correctly. The rules are well-documented and properly structured.
46-46: Verify handling of boundary cases for five-digit years.The pattern
/-?Y\d{5,}/correctly supports negative years, but let's verify the handling of boundary cases.Let's check the test coverage for five-digit years:
#!/bin/bash # Search for test cases involving five-digit years rg -A 3 "Y\d{5}" tests/ # Search for negative five-digit year tests rg -A 3 "-Y\d{5}" tests/
18-20: Verify handling of extreme negative years.The switch to SIGNED_INT for year values is correct. However, we should verify the handling of edge cases.
Let's check if there are any tests or validation for extreme negative years:
✅ Verification successful
Let me search for more specific test patterns and validation logic to verify the handling of extreme negative years.
Based on the search results, I can now generate the final response as we have found evidence of boundary handling for extreme years in the codebase.
Extreme negative years are properly handled with defined boundaries
The codebase implements explicit boundaries for year values:
- Minimum year limit is set to -2.5e16
- Maximum year limit is set to 2.5e16
- The implementation includes proper handling of year validation through min_year/max_year logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases involving negative years rg -l "test.*negative.*year|test.*year.*negative" tests/ # Search for actual negative year test values rg -A 2 "-\d{1,}.*year" tests/Length of output: 2205
Script:
#!/bin/bash # Search for test cases and validation logic related to negative years rg -e "test.*negative.*year" -e "test.*year.*negative" -e "-\d+.*year" tests/ -A 3 # Search for any validation or boundary checks on year values ast-grep --pattern 'if $_ < $_' ast-grep --pattern 'if $_ > $_' # Look for any year-related constants or limits rg -e "MIN.*YEAR|MAX.*YEAR|YEAR.*MIN|YEAR.*MAX" -e "year.*limit" -iLength of output: 1762
src/undate/dateformat/edtf/transformer.py (1)
69-78: Verify handling of edge cases.Let's ensure the implementation handles various edge cases correctly.
✅ Verification successful
Edge cases for year parsing are properly handled
The code's handling of edge cases for year parsing has been verified through the test suite and implementation:
- Negative years are correctly handled (e.g., "-Y17000002", "-1985")
- Large years with 5+ digits are supported (e.g., "Y17000002")
- The implementation properly:
- Detects negative years with
startswith("-")- Strips the "Y" prefix for extended year format
- Preserves the sign in the final integer conversion
The test files (particularly
tests/test_dateformat/edtf/test_edtf_transformer.pyandtests/test_dateformat/edtf/test_edtf_parser.py) demonstrate comprehensive coverage of these edge cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test cases covering edge cases in year parsing # Expected: Tests for various year formats including -Y00000, Y00000, -Y99999 # Look for test cases in the test files rg -A 5 "year_fivedigitsplus|test.*negative.*year" tests/Length of output: 59
Script:
#!/bin/bash # Let's try a broader search for test files and test cases related to year parsing # First, find test files related to EDTF date parsing fd "test.*\.py" tests/ --exec rg -l "edtf|date|year" # Then search for specific test cases with year values rg -A 5 "test.*year|Y\d{5}|-Y\d{4}|negative.*year" tests/Length of output: 5238
Script:
#!/bin/bash # Let's also check the implementation file for any validation or edge case handling rg -A 10 "def.*year|parse.*year|Y\d{5}|-Y\d{4}" src/Length of output: 4947
Script:
#!/bin/bash # Search for any error handling or validation patterns around year parsing ast-grep --pattern 'try { $$$ year = $$$ $$$ }' ast-grep --pattern 'if $COND { $$$ year = $$$ $$$ }'Length of output: 116
tests/test_dateformat/test_edtf.py (3)
16-20: LGTM! Improved test reliability for unknown dates.The change to use string comparison instead of direct object comparison is a good approach, as it correctly handles the semantics of unknown dates. The comments clearly explain the rationale.
27-27: LGTM! Simplified assertion reflects better EDTF understanding.The removal of the questioning comment and simplified assertion shows confidence in the EDTF support implementation.
53-57: Verify alignment with EDTF Level 2 specification.The test cases for unspecified digits look comprehensive. Let's verify they align with the official EDTF specification.
✅ Verification successful
Let me gather more specific information about the EDTF Level 2 implementation and specification details.
Let me check the actual implementation details of the EDTF format handling to verify the test cases.
Based on the implementation details and test cases shown, I can now generate the final response.
Test cases correctly implement EDTF Level 2 unspecified digit patterns
The test cases align with EDTF Level 2 specification for unspecified digits:
- Supports partial unspecified year digits (e.g., "156X")
- Allows fully unspecified years ("XXXX")
- Handles unspecified month digits (e.g., "1XXX-XX")
- Supports unspecified day components
- Implementation correctly converts between internal representation and EDTF format
The implementation in
EDTFDateFormatclass properly handles the conversion usingEDTF_UNSPECIFIED_DIGITconstant and maintains the format specification requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EDTF specification references in documentation or comments rg -i "edtf.*level.*2|extended.*date.*time.*format.*level.*2" --type md --type txt # Look for any existing level 2 feature documentation fd -e md -e rst -e txt -e py | xargs rg "unspecified.*digits"Length of output: 633
Script:
#!/bin/bash # Check the parser implementation for unspecified digits ast-grep --pattern 'def parse_unspecified_digits($$$)' # Look for EDTF specification or documentation comments rg -B 3 -A 10 "level 2 unspecified digits" # Check the transformer implementation ast-grep --pattern 'def transform_unspecified_digits($$$)' # Look for any documentation about EDTF format support fd README -e md | xargs rg -i "edtf"Length of output: 3177
Script:
#!/bin/bash # Check the EDTF format implementation rg -l "class.*EDTFDateFormat" | xargs cat # Look for unspecified digit handling in the implementation rg -B 2 -A 5 "def.*to_string" src/ # Check for any EDTF specification references in comments rg -B 2 -A 5 "EDTF.*Level.*2|Extended.*Date.*Time.*Format.*Level.*2" -iLength of output: 9700
src/undate/dateformat/edtf/formatter.py (1)
56-62: LGTM! The negative year handling looks correct.The implementation correctly handles the EDTF specification requirement for years with more than 4 digits while properly preserving the negative sign. The logic appropriately:
- Excludes the minus sign when counting digits
- Preserves the negative sign in the final output
- Adds the 'Y' prefix as required by the EDTF spec
Let's verify the handling of various year formats:
✅ Verification successful
The negative year handling implementation is well-tested and correctly follows EDTF specification
The test files show comprehensive coverage for the negative year handling:
- Test cases explicitly verify formatting of large years:
"Y17000002"and"-Y17000002"- Negative year tests include both regular 4-digit years (
"-1985") and extended years- The
test_to_stringmethod inTestEDTFDateFormatspecifically verifies:
Undate(17000002)→"Y17000002"Undate(-17000002)→"-Y17000002"Undate(-1985)→"-1985"The implementation correctly handles all test cases and follows the EDTF specification requirements for both regular and extended year formats.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for various year formats # Expected: Find test cases covering negative years and years > 4 digits # Test: Search for relevant test cases rg -A 5 "test.*year" "tests/test_dateformat/test_edtf.py" # Test: Check if there are any existing EDTF examples in documentation fd -g "*.md" -x rg -l "EDTF.*year"Length of output: 94
Script:
#!/bin/bash # Let's try a different approach to find test cases and examples # Search for test files related to EDTF fd -g "test_edtf*.py" -x cat {} # Search for any year-related patterns in test files rg -l "Y\d{5,}|-\d{4,}" tests/ # Look for EDTF formatter test class and its methods ast-grep --pattern 'class Test$_EDTF { $$$ }' # Check the actual formatter implementation for context cat src/undate/dateformat/edtf/formatter.pyLength of output: 9540
src/undate/undate.py (2)
57-59: LGTM: Special case handling for unknown year.The implementation correctly handles "XXXX" as an unknown year according to EDTF specification.
Line range hint
116-122: Verify the robustness of max day calculation.The logic for calculating the maximum day has been improved to handle unknown years, but there are a few considerations:
- Using a non-leap year constant (
self.NON_LEAP_YEAR) for unknown years means February will always be calculated as 28 days.- The fallback to 31 days could be too permissive for some months.
Let's verify the impact of this change on date calculations:
Consider enhancing the logic to handle leap years more accurately:
elif year is None and month: - # If we don't have year and month, - # calculate based on a known non-leap year - # (better than just setting 31, but still not great) - _, max_day = monthrange(self.NON_LEAP_YEAR, max_month) + # For unknown years, use the maximum possible days for the month + # For February, this means 29 days to account for leap years + if max_month == 2: + max_day = 29 + else: + _, max_day = monthrange(self.NON_LEAP_YEAR, max_month)examples/notebooks/edtf-support.ipynb (1)
773-789: Consider documenting Python version requirements.The notebook uses Python 3.13.0, which is a very recent version. Consider:
- Documenting minimum Python version requirements
- Testing compatibility with older Python versions (e.g., 3.8+)
- Adding version checks in the code
| "import datetime \n", | ||
| "\n", | ||
| "from undate import Undate, UndateInterval, DatePrecision\n", | ||
| "\n", | ||
| "# Example 1: day\n", | ||
| "day = Undate.parse(\"1985-04-12\", \"EDTF\")\n", | ||
| "assert day.precision == DatePrecision.DAY\n", | ||
| "assert day == datetime.date(1985, 4, 12)\n", | ||
| "\n", | ||
| "# Example 2 : month\n", | ||
| "month = Undate.parse(\"1985-04\", \"EDTF\")\n", | ||
| "assert month.year == \"1985\" and month.month == \"04\"\n", | ||
| "assert month.precision == DatePrecision.MONTH\n", | ||
| "\n", | ||
| "# Example 3 : year\n", | ||
| "year = Undate.parse(\"1985\", \"EDTF\")\n", | ||
| "assert year.year == \"1985\"\n", | ||
| "assert year.precision == DatePrecision.YEAR" | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "markdown", | ||
| "id": "6666c12d-7fda-419a-bbd9-af68ed4bbff0", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "#### Output in EDTF format\n", | ||
| "\n", | ||
| "Demonstrate that initalizing `Undate` objects and serializing with EDTF formatter returns the expected value." | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "code", | ||
| "execution_count": 2, | ||
| "id": "923476ff-344a-4018-a02e-6e5f80ea76a8", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "from undate.undate import Undate, DatePrecision\n", | ||
| "from undate.dateformat.edtf import EDTFDateFormat\n", | ||
| "\n", | ||
| "# set default format to EDTF\n", | ||
| "Undate.DEFAULT_FORMAT = \"EDTF\"\n", | ||
| "\n", | ||
| "# Example 1: day\n", | ||
| "day = Undate(1985, 4, 12)\n", | ||
| "# confirm EDTF formatter is being used\n", | ||
| "assert isinstance(day.formatter, EDTFDateFormat)\n", | ||
| "# casting to str is now equivalent to day.format(\"EDTF\")\n", | ||
| "assert str(day) == \"1985-04-12\"\n", | ||
| "assert day.precision == DatePrecision.DAY\n", | ||
| "\n", | ||
| "# Example 2 : month\n", | ||
| "month = Undate(1985, 4)\n", | ||
| "assert str(month) == \"1985-04\"\n", | ||
| "assert month.precision == DatePrecision.MONTH\n", | ||
| "\n", | ||
| "# Example 3 : year\n", | ||
| "year = Undate(1985)\n", | ||
| "assert str(year) == \"1985\"\n", | ||
| "assert year.precision == DatePrecision.YEAR" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error case handling.
While the happy path test cases are thorough, consider adding tests for error cases such as:
- Invalid dates (e.g., "1985-13-01", "1985-04-31")
- Malformed input strings
- Invalid precision combinations
![kypso-io[bot]](https://avatars.githubusercontent.com/in/843074?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
- Number of files reviewed: 9
- Number of comments: 0
- Number of suggestions: 0
📚 File changes
| File path | File changes |
|---|---|
| examples/notebooks/edtf-support.ipynb | Introduced a Jupyter notebook demonstrating the undate library's support for EDTF, with enhanced handling of negative years and unspecified digits. |
| src/undate/init.py | Added imports for Undate, UndateInterval, and DatePrecision. |
| src/undate/dateformat/edtf/edtf.lark | Updated the grammar to support signed integers for years, added handling for unspecified years, and improved regex for five-digit years. |
| src/undate/dateformat/edtf/formatter.py | Refactored year handling logic to include negative year support. |
| src/undate/dateformat/edtf/transformer.py | Refactored the year extraction logic in the year_fivedigitsplus method. |
| src/undate/undate.py | Refactored handling of year and month in the Undate class to improve clarity and robustness. |
| tests/test_dateformat/edtf/test_edtf_parser.py | Added test cases for negative years and unspecified digits. |
| tests/test_dateformat/edtf/test_edtf_transformer.py | Added test cases for negative years and unspecified digits in date parsing. |
| tests/test_dateformat/test_edtf.py | Updated test cases for parsing and formatting in the EDTFDateFormat class. |
Ignored comments
examples/notebooks/edtf-support.ipynb
- refactor_suggestion: The notebook structure is well organized, but consider breaking down large code cells into smaller, more focused cells. This can improve readability and make it easier to test individual components. For example, each example could be placed in its own cell with a brief explanation of what it demonstrates.
src/undate/init.py
- refactor_suggestion: Consider organizing imports in a more structured way, such as grouping standard library imports, third-party imports, and local application imports separately. This can enhance readability and maintainability of the code.
src/undate/dateformat/edtf/edtf.lark
-
refactor_suggestion: The import of
SIGNED_INTis now included, which is a good change for handling signed integers. However, ensure that all instances whereyearis used are updated to reflect that it can now be negative. This may require additional validation or handling in other parts of the code that interact with this grammar definition. -
refactor_suggestion: The change from
INTtoSIGNED_INTfor theyeardefinition is appropriate for supporting negative years. Ensure that any logic that processesyearvalues is also updated to handle negative values correctly, especially in date calculations or comparisons. -
refactor_suggestion: The addition of the comment about Level 2 and unspecified years is helpful for understanding the context. Ensure that the implementation correctly handles cases where the year is completely unspecified, as this could lead to ambiguities in date parsing.
-
refactor_suggestion: The regex for
year_fivedigitsplushas been updated to allow for negative years, which is a necessary change. Make sure that any related logic that processes this regex is also aware of the potential for negative values, particularly in validation or formatting functions.
src/undate/dateformat/edtf/formatter.py
- refactor_suggestion: The logic for handling negative years has been added, which improves clarity and correctness. However, consider extracting the logic for handling the year prefixing into a separate method to enhance readability and maintainability.
def _format_year(self, year: str) -> str:
negative_year = ""
if year.startswith("-"):
negative_year = "-"
year = year[1:]
return f"{negative_year}Y{year}"
if undate.precision >= DatePrecision.YEAR:
year = self._convert_missing_digits(undate.year, undate.MISSING_DIGIT)
if year and len(year.lstrip("-")) > 4:
year = self._format_year(year)
parts.append(year or EDTF_UNSPECIFIED_DIGIT * 4)
src/undate/dateformat/edtf/transformer.py
- refactor_suggestion: The logic for checking if the year is negative and stripping the leading 'Y' can be encapsulated into a separate method for better readability and reusability. Consider creating a helper function that handles the extraction and sign determination of the year.
def extract_year(self, token):
value = token.value
negative = False
if value.startswith("-"):
value = value[1:]
negative = True
year = int(value.lstrip("Y"))
return -year if negative else year
def year_fivedigitsplus(self, items):
# strip off the leading Y and convert to integer
token = items[0]
year = self.extract_year(token)
return Tree(data="year", children=[year])
src/undate/undate.py
-
refactor_suggestion: The comment about refactoring partial date min/max calculations has been replaced with a more specific comment regarding treating 'XXXX' as unknown. Consider removing the old comment if it is no longer relevant, or updating it to reflect the current state of the code.
-
refactor_suggestion: The condition checking if
yearandmonthare present has been made more explicit by ensuringyearis an integer. This improves clarity and prevents potential issues with type mismatches. Ensure that this change is consistent with the rest of the codebase where similar checks are performed.
tests/test_dateformat/edtf/test_edtf_parser.py
- refactor_suggestion: Consider using a more structured approach to categorize test cases, such as using dictionaries or classes to group related test cases together. This can improve readability and maintainability.
tests/test_dateformat/edtf/test_edtf_transformer.py
-
refactor_suggestion: Consider grouping the test cases logically, such as separating valid dates, negative years, and unspecified digits for better organization and readability.
-
refactor_suggestion: It may be beneficial to add comments or section headers to clarify the purpose of each group of test cases, especially for complex formats like unspecified digits and intervals.
tests/test_dateformat/test_edtf.py
- refactor_suggestion: The comment regarding the comparison of unknown dates could be more concise. Consider simplifying the explanation to improve clarity.
comparison doesn't work because undate knows unknown dates aren't necessarily the same, so use string comparison
assert str(EDTFDateFormat().parse("XXXX-05-03")) == Undate(month=5, day=3).format("EDTF")
- refactor_suggestion: The commented-out line regarding the parsing logic could be removed or clarified. If it is not relevant, it may be better to delete it to avoid confusion.
assert EDTFDateFormat().parse("XXXX-05-03") != Undate(month=5, day=4)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #98 +/- ##
===========================================
- Coverage 98.04% 97.72% -0.33%
===========================================
Files 10 10
Lines 460 483 +23
===========================================
+ Hits 451 472 +21
- Misses 9 11 +2 ☔ View full report in Codecov by Sentry. |
work towards #73
Summary by CodeRabbit
Release Notes
New Features
edtf-support.ipynb) demonstrating theundatelibrary's support for the Extended Date/Time Format (EDTF).Bug Fixes
Tests