Make EDTF parser available as undate formatter; handle 5+ digit years #89
Conversation
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
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 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 (
|
![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: 10
- Number of comments: 5
- Number of suggestions: 4
📚 File changes
| File path | File changes |
|---|---|
| src/undate/date.py | Updated comments and rearranged precision constants in the DatePrecision class. |
| src/undate/dateformat/edtf/init.py | Added import statement for EDTFDateFormat from the formatter module. |
| src/undate/dateformat/edtf/formatter.py | Added EDTF parser as undate formatter and handled 5+ digit years. |
| src/undate/dateformat/edtf/transformer.py | Updated the year_fivedigitsplus method to accept items instead of token and modified the logic to handle 5+ digit years. |
| src/undate/dateformat/iso8601.py | Removed duplicate import statement for typing. |
| src/undate/undate.py | Added validation TODO, fixed comparison logic for precision, and refactored date part retrieval methods. |
| tests/test_date.py | Added tests for DatePrecision comparisons and modified test structure for Date initialization. |
| tests/test_dateformat/edtf/test_edtf_transformer.py | Updated test cases to include support for 5+ digit years. |
| tests/test_dateformat/test_edtf.py | Added tests for parsing and formatting 5+ digit years in the EDTFDateFormat. |
| tests/test_undate.py | Added tests for year, month, and day properties of the Undate class. |
src/undate/undate.py
Outdated
| # is precision sufficient for comparing partially known dates? | ||
| self.precision > other.precision, | ||
| self.precision < other.precision, |
There was a problem hiding this comment.
The comparison logic for precision should be corrected. The original line checks if self.precision is greater than other.precision, which may lead to incorrect behavior. It should be less than instead, as indicated by the change in the diff.
src/undate/date.py
Outdated
There was a problem hiding this comment.
Made the same comment over in the numpy PR #84, but wanted to flag here too:
One potential issue with ordering it this way is that it's going to be harder to extend. I guess we could do DECADE = 0? My understanding is that we can't get precise than DAY (since we're not recording times) but there are potentially future cases where we might want to be less precise than YEAR?
tests/test_undate.py
Outdated
There was a problem hiding this comment.
Having trouble testing this locally. Recreated venv after testing the numpy branch, but I'm getting ModuleNotFoundErrors: Hint: make sure your test modules/packages have valid Python names.. Guessing this will resolve if #84 is rebased in here?
It is branched off from that but will probably need to be updated, they may have diverged by now. I'll update this one after the big numpy refactor is finally merged into develop. |
There was a problem hiding this comment.
📋 Review Summary
- Number of files reviewed: 10
- Number of comments: 3
- Number of suggestions: 2
📚 File changes
| File path | File changes |
|---|---|
| src/undate/date.py | Updated comments for clarity and corrected the order of precision constants. |
| src/undate/dateformat/edtf/init.py | Added import statement for EDTFDateFormat from the formatter module. |
| src/undate/dateformat/edtf/formatter.py | Added EDTF parser as undate formatter and support for 5+ digit years. |
| src/undate/dateformat/edtf/transformer.py | Changed the parameter of year_fivedigitsplus from token to items and updated the implementation to handle 5+ digit years. |
| src/undate/dateformat/iso8601.py | Removed duplicate import statement for typing. |
| src/undate/undate.py | Added validation for expected string lengths, modified comparison logic for date precision, and refactored date part retrieval methods. |
| tests/test_date.py | Added tests for DatePrecision comparisons and modified test method names. |
| tests/test_dateformat/edtf/test_edtf_transformer.py | Updated test cases and comments related to level 1 functionality and year support. |
| tests/test_dateformat/test_edtf.py | Added tests for parsing and formatting 5+ digit years in the EDTFDateFormat. |
| tests/test_undate.py | Added tests for properties accessing parts of the date, including year, month, and day. |
Ignored comments
src/undate/date.py
- refactor_suggestion: The comments regarding the precision of the date units should be consistent and clear. The previous comment indicated that year precision is greater than month precision, which is misleading. It would be better to clarify the relationship between the different precisions without ambiguity.
comparison, e.g. year is LESS precise than month
numbers should be set to allow logical greater than / less than
e.g. YEAR < MONTH < DAY
- refactor_suggestion: The order of the precision constants should be corrected to reflect their actual precision levels. The
YEARconstant should be set to 1,MONTHto 2, andDAYto 3, which is already correctly represented in the new lines. The old lines should be removed to avoid confusion.
YEAR = 1
MONTH = 2
DAY = 3
src/undate/dateformat/edtf/init.py
- refactor_suggestion: Consider adding an
__all__variable to explicitly declare the public interface of this module. This can improve clarity about what is intended to be part of the public API.
src/undate/dateformat/edtf/formatter.py
- refactor_suggestion: The
to_stringmethod contains several TODO comments regarding handling uncertain or approximate dates. It would be better to implement this functionality or at least provide a clear mechanism for handling these cases to avoid leaving the method incomplete.
# Implement handling for uncertain / approximate dates here
# For example, you could check for specific flags or conditions
src/undate/dateformat/edtf/transformer.py
- refactor_suggestion: The method
year_fivedigitsplushas been changed to acceptitemsinstead oftoken, which is a good improvement for consistency. However, the comment about the limitation of 4-digit years should be removed since the functionality now supports 5+ digit years. Additionally, consider renaming the method to reflect its capability to handle 5+ digit years more clearly.
src/undate/dateformat/iso8601.py
- refactor_suggestion: The import statement for
Dict,List, andUnionis duplicated. It should be removed from the second occurrence to maintain clean code and avoid redundancy.
src/undate/undate.py
-
refactor_suggestion: The TODO comment suggests adding validation for string lengths, which is important for ensuring that the input values conform to expected formats. Consider implementing this validation to prevent potential issues with unexpected string lengths during parsing.
-
refactor_suggestion: The methods for retrieving year, month, and day could be refactored to reduce redundancy. Consider creating a single method that handles the formatting and retrieval of date parts to streamline the code and improve maintainability.
tests/test_date.py
-
refactor_suggestion: The test methods should follow a consistent naming convention to improve clarity. Consider renaming
test_init_year_month_daytotest_init_year_monthto accurately reflect the parameters being tested. -
refactor_suggestion: The
test_comparisonmethod could be better organized by grouping related assertions together. This will enhance readability and maintainability of the test cases.
tests/test_dateformat/edtf/test_edtf_transformer.py
- refactor_suggestion: The comments regarding the limitations of the
undatefunctionality have been modified. It is important to ensure that any comments accurately reflect the current capabilities of the code. Ifundatenow supports years beyond 9999, the comment should be updated to reflect that. Otherwise, it may mislead future developers about the functionality.
tests/test_undate.py
-
refactor_suggestion: The assertions for the
monthproperty on line 149 should use==instead ofisfor string comparison. Theisoperator checks for identity, not equality, which can lead to unexpected results when comparing strings.
assert Undate(2023, "XX").month == "XX" -
refactor_suggestion: The assertions for the
dayproperty on line 163 should use==instead ofisfor string comparison. Theisoperator checks for identity, not equality, which can lead to unexpected results when comparing strings.
assert Undate(2023, 1, "XX").day == "XX"
This adds glue between undate and the existing EDTF parsing by adding a date formatter class. The parse method simply uses the parser and transformer; the to string method uses custom logic because I couldn't figure out an elegant way to leverage the parsing code (there's a reconstruct method but the tree is pretty complicated).
I also added support for parsing and formatting 5+ digit years now that we support it (this is based on the numpy code in #84 ).