Repository navigation
feat: add xsd:duration codec and data type - #64
Conversation
Summary of ChangesHello @mark-dropbear, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the library by introducing full support for the xsd:duration data type. This involves creating a new XsdDuration class that accurately models the XSD specification for durations, which accounts for both year/month and day/time components, and handles their distinct behaviors. A corresponding codec facilitates seamless conversion between string representations and XsdDuration objects, while comprehensive tests validate the implementation's correctness and adherence to the standard. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the xsd:duration data type, complete with a new XsdDuration class, a codec for serialization/deserialization, and a comprehensive set of unit tests. The implementation correctly models the dual-component nature of XSD durations (months and seconds) and effectively handles parsing from and formatting to the canonical string representation. The code is well-organized, and the tests cover a wide range of valid, invalid, and edge-case scenarios, which is excellent. I have a few suggestions for minor code cleanup in the XsdDuration class to enhance clarity and maintainability. Overall, this is a very well-executed feature addition.
| static XsdDuration parse(String lexicalForm) { | ||
| // Regex for Duration: | ||
| // -?P((([0-9]+Y)?([0-9]+M)?([0-9]+D)?(T([0-9]+H)?([0-9]+M)?([0-9]+(\.[0-9]+)?S)?)?)|(T([0-9]+H)?([0-9]+M)?([0-9]+(\.[0-9]+)?S)?)) | ||
| // Simplified: -?P... | ||
| // We will parse manually or with a simpler regex to extract parts. | ||
|
|
||
| if (lexicalForm.isEmpty) { | ||
| throw FormatException('Invalid duration format: empty string'); | ||
| } | ||
|
|
||
| bool isNegative = false; | ||
| String input = lexicalForm; | ||
| if (input.startsWith('-')) { | ||
| isNegative = true; | ||
| input = input.substring(1); | ||
| } | ||
|
|
||
| if (!input.startsWith('P')) { | ||
| throw FormatException( | ||
| 'Invalid duration format: must start with P (after optional sign)', | ||
| lexicalForm, | ||
| ); | ||
| } | ||
|
|
||
| // Check for empty P (and potentially empty T) | ||
| if (input == 'P') { | ||
| throw FormatException( | ||
| 'Invalid duration format: no components specified', | ||
| lexicalForm, | ||
| ); | ||
| } | ||
|
|
||
| // Split into Date and Time parts | ||
| final parts = input.substring(1).split('T'); | ||
| if (parts.length > 2) { | ||
| throw FormatException( | ||
| 'Invalid duration format: multiple T separators', | ||
| lexicalForm, | ||
| ); | ||
| } | ||
|
|
||
| String datePart = parts[0]; | ||
| String timePart = parts.length > 1 ? parts[1] : ''; | ||
|
|
||
| if (parts.length > 1 && timePart.isEmpty) { | ||
| throw FormatException( | ||
| 'Invalid duration format: T present but no time components', | ||
| lexicalForm, | ||
| ); | ||
| } | ||
|
|
||
| // We will consume the string from left to right. | ||
|
|
||
| int parsedMonths = 0; | ||
| Decimal parsedSeconds = Decimal.zero; | ||
|
|
||
| // Parse Date Part | ||
| if (datePart.isNotEmpty) { | ||
| // Y | ||
| int yIndex = datePart.indexOf('Y'); | ||
| if (yIndex != -1) { | ||
| String val = datePart.substring(0, yIndex); | ||
| parsedMonths += int.parse(val) * 12; | ||
| datePart = datePart.substring(yIndex + 1); | ||
| } | ||
|
|
||
| // M | ||
| int mIndex = datePart.indexOf('M'); | ||
| if (mIndex != -1) { | ||
| String val = datePart.substring(0, mIndex); | ||
| parsedMonths += int.parse(val); | ||
| datePart = datePart.substring(mIndex + 1); | ||
| } | ||
|
|
||
| // D | ||
| int dIndex = datePart.indexOf('D'); | ||
| if (dIndex != -1) { | ||
| String val = datePart.substring(0, dIndex); | ||
| parsedSeconds += Decimal.parse(val) * Decimal.fromInt(86400); | ||
| datePart = datePart.substring(dIndex + 1); | ||
| } | ||
|
|
||
| if (datePart.isNotEmpty) { | ||
| throw FormatException( | ||
| 'Invalid duration format: unexpected characters in date part "$datePart"', | ||
| lexicalForm, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Parse Time Part | ||
| if (timePart.isNotEmpty) { | ||
| // H | ||
| int hIndex = timePart.indexOf('H'); | ||
| if (hIndex != -1) { | ||
| String val = timePart.substring(0, hIndex); | ||
| parsedSeconds += Decimal.parse(val) * Decimal.fromInt(3600); | ||
| timePart = timePart.substring(hIndex + 1); | ||
| } | ||
|
|
||
| // M | ||
| int mIndex = timePart.indexOf('M'); | ||
| if (mIndex != -1) { | ||
| String val = timePart.substring(0, mIndex); | ||
| parsedSeconds += Decimal.parse(val) * Decimal.fromInt(60); | ||
| timePart = timePart.substring(mIndex + 1); | ||
| } | ||
|
|
||
| // S | ||
| int sIndex = timePart.indexOf('S'); | ||
| if (sIndex != -1) { | ||
| String val = timePart.substring(0, sIndex); | ||
| parsedSeconds += Decimal.parse(val); | ||
| timePart = timePart.substring(sIndex + 1); | ||
| } | ||
|
|
||
| if (timePart.isNotEmpty) { | ||
| throw FormatException( | ||
| 'Invalid duration format: unexpected characters in time part "$timePart"', | ||
| lexicalForm, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (isNegative) { | ||
| parsedMonths = -parsedMonths; | ||
| parsedSeconds = -parsedSeconds; | ||
| } | ||
|
|
||
| return XsdDuration(months: parsedMonths, seconds: parsedSeconds); | ||
| } |
There was a problem hiding this comment.
The parse method is quite long, containing all the logic for parsing the duration string. To improve readability and maintainability, consider refactoring this method by extracting the logic for parsing the date and time components into separate private helper methods. For example, you could have _parseDatePart and _parseTimePart methods. This would make the main parse method's control flow clearer and each part's logic more focused.
| if (s.contains('.') && s.endsWith('0')) { | ||
| // Decimal package toString handles this well usually, but let's be safe if needed. | ||
| // Actually Decimal.toString() is usually good. | ||
| } |
There was a problem hiding this comment.
This if block is empty and its associated comments suggest some uncertainty that is no longer relevant. Since the decimal package's toString() method handles the canonical representation correctly, this block is unnecessary and can be removed to improve code clarity.
| } else if (years == 0 && monthPart == 0 && days == BigInt.zero) { | ||
| // Should have been handled by the zero check at start, but if we have 0 seconds but non-zero months/days, we don't write T. | ||
| // If we are here, it means we had some months/days, so we don't need T part. | ||
| } |
There was a problem hiding this comment.
This else if block is empty and appears to be unreachable code. The case where all duration components are zero is already handled at the beginning of the toString() method. This block can be safely removed to simplify the implementation.
1 participant
No description provided.