Repository navigation
feat: add xsd:datetime codec and data type - #65
mark-dropbear wants to merge 3 commits into
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's support for XSD 1.1 built-in datatypes by introducing the 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:datetime data type by adding a new XsdDateTime class and a corresponding XsdDateTimeCodec. The implementation correctly handles floating, UTC, and offset-based date-time strings, and includes logic to preserve the original timezone offset for round-tripping. The code is well-structured and includes a comprehensive set of unit tests.
My review includes a few suggestions for improvement:
- In
xsd_datetime.dart, I've suggested improving the documentation for theparsemethod, optimizing performance by caching regular expressions, and refining thetoString()method to better handle round-tripping of values without fractional seconds. - In
README.md, I've proposed a clarification to the note aboutDateTime's timezone handling. - I've also noted that the tests for
toString()will need to be updated if the implementation is changed as suggested. - Finally, a minor formatting fix for
CHANGELOG.mdis included.
Overall, this is a great addition to the library.
| /// Parses an XSD dateTime string. | ||
| /// | ||
| /// Handles formats like: | ||
| /// - `2002-10-10T12:00:00` (Floating) | ||
| /// - `2002-10-10T12:00:00Z` (UTC) | ||
| /// - `2002-10-10T12:00:00-05:00` (Offset) |
There was a problem hiding this comment.
The documentation for parse is helpful, but it would be even better to explicitly mention that it can throw a FormatException for invalid input, which is standard practice for parse methods in Dart.
/// Parses an XSD dateTime string.
///
/// Throws a [FormatException] if the [input] is not a valid representation.
///
/// Handles formats like:
/// - `2002-10-10T12:00:00` (Floating)
/// - `2002-10-10T12:00:00Z` (UTC)
/// - `2002-10-10T12:00:00-05:00` (Offset)| static XsdDateTime parse(String input) { | ||
| // Regex to detect timezone presence. | ||
| // Matches Z or +/-HH:MM at the end. | ||
| final hasTimezone = RegExp(r'(Z|[+-]\d{2}:\d{2})$').hasMatch(input); | ||
|
|
||
| if (!hasTimezone) { | ||
| // Floating time. Parse as if it were UTC to avoid local timezone interference. | ||
| // We append 'Z' to force DateTime.parse to treat it as UTC, then strip it conceptually. | ||
| // Actually, DateTime.parse handles '2002-10-10T12:00:00' as local. | ||
| // To ensure we store the exact values provided without local conversion, | ||
| // we can append 'Z' to the input string before parsing. | ||
| final utcInput = '${input}Z'; | ||
| final dt = DateTime.parse(utcInput); | ||
| return XsdDateTime(dt, isFloating: true); | ||
| } else { | ||
| final dt = DateTime.parse(input); | ||
| Duration? offset; | ||
|
|
||
| if (input.endsWith('Z')) { | ||
| offset = Duration.zero; | ||
| } else { | ||
| // Extract offset manually because DateTime doesn't expose the original offset | ||
| // if it converts to UTC/Local. | ||
| // Format is +/-HH:MM | ||
| final match = RegExp(r'([+-])(\d{2}):(\d{2})$').firstMatch(input); | ||
| if (match != null) { | ||
| final sign = match.group(1) == '+' ? 1 : -1; | ||
| final hours = int.parse(match.group(2)!); | ||
| final minutes = int.parse(match.group(3)!); | ||
| offset = Duration(hours: hours, minutes: minutes) * sign; | ||
| } | ||
| } | ||
|
|
||
| return XsdDateTime(dt, isFloating: false, originalOffset: offset); | ||
| } | ||
| } |
There was a problem hiding this comment.
The regular expressions on lines 39 and 60 are recompiled on every call to parse. For better performance, you can define them as static final fields on the XsdDateTime class to compile them only once.
Example:
class XsdDateTime implements Comparable<XsdDateTime> {
static final _timezoneRegex = RegExp(r'(Z|[+-]\d{2}:\d{2})$');
static final _offsetRegex = RegExp(r'([+-])(\d{2}):(\d{2})$');
// ... other members
static XsdDateTime parse(String input) {
final hasTimezone = _timezoneRegex.hasMatch(input);
// ...
}
// ...
}| String toString() { | ||
| if (isFloating) { | ||
| // Return ISO string without 'Z' or offset. | ||
| // value is UTC, so toIso8601String() returns ...Z | ||
| return value.toIso8601String().replaceAll('Z', ''); | ||
| } | ||
|
|
||
| if (originalOffset != null && originalOffset != Duration.zero) { | ||
| // We need to shift the UTC value back to the original offset for display. | ||
| final localTime = value.add(originalOffset!); | ||
| // iso is ...Z or ... depending on how it was created, but since we added offset manually | ||
| // to a UTC date, the result is technically "what the time would be in that zone". | ||
| // However, DateTime.toIso8601String() always ends in Z if isUtc is true, | ||
| // or nothing if isUtc is false. | ||
| // Since 'value' is UTC, 'localTime' is also UTC (just shifted). | ||
| // We want to strip the Z and append the offset. | ||
|
|
||
| // Remember that DateTime.add returns a DateTime with the same isUtc property. | ||
| // So localTime is still UTC. | ||
| // Example: 12:00Z + 5 hours = 17:00Z. | ||
| // We want to print 17:00+05:00. | ||
| final bareIso = localTime.toIso8601String().replaceAll('Z', ''); | ||
|
|
||
| final sign = originalOffset!.isNegative ? '-' : '+'; | ||
| final absOffset = originalOffset!.abs(); | ||
| final hours = absOffset.inHours.toString().padLeft(2, '0'); | ||
| final minutes = (absOffset.inMinutes % 60).toString().padLeft(2, '0'); | ||
|
|
||
| return '$bareIso$sign$hours:$minutes'; | ||
| } | ||
|
|
||
| return value.toIso8601String(); | ||
| } |
There was a problem hiding this comment.
The current toString() implementation uses toIso8601String() which always includes milliseconds, even if they are zero (e.g., .000). This means that parsing a string like 2002-10-10T12:00:00 and then converting it back to a string results in 2002-10-10T12:00:00.000, which is not identical to the original input. While valid, this can be undesirable for round-tripping lexical representations.
To better preserve the original format, consider modifying toString() to omit the fractional part if it's zero. You could introduce a helper function that checks if millisecond and microsecond are zero and, if so, removes the .000 part from the toIso8601String() output before further processing.
| import 'package:test/test.dart'; | ||
| import 'package:xsd/src/types/xsd_datetime.dart'; | ||
|
|
||
| void main() { | ||
| group('XsdDateTime', () { | ||
| test('parses floating time', () { | ||
| final dt = XsdDateTime.parse('2002-10-10T12:00:00'); | ||
| expect(dt.isFloating, isTrue); | ||
| expect(dt.value.isUtc, isTrue); | ||
| expect(dt.value.year, 2002); | ||
| expect(dt.value.month, 10); | ||
| expect(dt.value.day, 10); | ||
| expect(dt.value.hour, 12); | ||
| expect(dt.value.minute, 0); | ||
| expect(dt.value.second, 0); | ||
| expect(dt.originalOffset, isNull); | ||
| expect(dt.toString(), '2002-10-10T12:00:00.000'); | ||
| }); | ||
|
|
||
| test('parses UTC time', () { | ||
| final dt = XsdDateTime.parse('2002-10-10T12:00:00Z'); | ||
| expect(dt.isFloating, isFalse); | ||
| expect(dt.value.isUtc, isTrue); | ||
| expect(dt.originalOffset, Duration.zero); | ||
| expect(dt.toString(), '2002-10-10T12:00:00.000Z'); | ||
| }); | ||
|
|
||
| test('parses offset time', () { | ||
| final dt = XsdDateTime.parse('2002-10-10T12:00:00-05:00'); | ||
| expect(dt.isFloating, isFalse); | ||
| expect(dt.value.isUtc, isTrue); | ||
| // 12:00 -05:00 is 17:00 UTC | ||
| expect(dt.value.hour, 17); | ||
| expect(dt.originalOffset, const Duration(hours: -5)); | ||
| expect(dt.toString(), '2002-10-10T12:00:00.000-05:00'); | ||
| }); | ||
|
|
||
| test('parses positive offset time', () { | ||
| final dt = XsdDateTime.parse('2002-10-10T12:00:00+05:30'); | ||
| expect(dt.isFloating, isFalse); | ||
| expect(dt.value.isUtc, isTrue); | ||
| // 12:00 +05:30 is 06:30 UTC | ||
| expect(dt.value.hour, 6); | ||
| expect(dt.value.minute, 30); | ||
| expect(dt.originalOffset, const Duration(hours: 5, minutes: 30)); | ||
| expect(dt.toString(), '2002-10-10T12:00:00.000+05:30'); | ||
| }); | ||
|
|
||
| test('equality', () { | ||
| final dt1 = XsdDateTime.parse('2002-10-10T12:00:00'); | ||
| final dt2 = XsdDateTime.parse('2002-10-10T12:00:00'); | ||
| final dt3 = XsdDateTime.parse('2002-10-10T12:00:00Z'); | ||
|
|
||
| expect(dt1, equals(dt2)); | ||
| expect(dt1.hashCode, equals(dt2.hashCode)); | ||
| expect(dt1, isNot(equals(dt3))); // Floating != Zoned | ||
| expect(dt1.hashCode, isNot(equals(dt3.hashCode))); | ||
| }); | ||
|
|
||
| test('compareTo', () { | ||
| final dt1 = XsdDateTime.parse( | ||
| '2002-10-10T12:00:00', | ||
| ); // Floating (treated as UTC 12:00) | ||
| final dt2 = XsdDateTime.parse('2002-10-10T13:00:00Z'); // UTC 13:00 | ||
|
|
||
| expect(dt1.compareTo(dt2), lessThan(0)); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The assertions for toString() (e.g., on lines 17, 25, 35, 46) expect the output to include .000 for fractional seconds, even when the input string did not have them. This is tied to the current toString() implementation in XsdDateTime.
As noted in a separate comment on xsd_datetime.dart, this behavior could be improved for better round-tripping of lexical values by omitting zero-valued fractional seconds. If that change is made, these tests will need to be updated to reflect the new expected output (e.g., 2002-10-10T12:00:00 instead of 2002-10-10T12:00:00.000).
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
No description provided.