Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Constructor DateTime(const char *iso8601dateTime) is not compliant with iso8601 times. #274

Closed
matdon opened this issue Feb 9, 2023 · 2 comments · May be fixed by #275
Closed

Constructor DateTime(const char *iso8601dateTime) is not compliant with iso8601 times. #274

matdon opened this issue Feb 9, 2023 · 2 comments · May be fixed by #275

Comments

@matdon
Copy link

matdon commented Feb 9, 2023 •
edited
Loading

The constructor DateTime(const char *iso8601dateTime) claims to enable the creation of DateTime objects using an ISO8601 date time definition.

However, the constructor only works for iso datetimes of format:
"2000-01-01T00:00:00" that can be truncated from the right at an arbitrary position, e.g. the string "2000-01" leads to a valid result.

This can be seen from the constructor implementation in the code:

  DateTime::DateTime(const char *iso8601dateTime) {
    char ref[] = "2000-01-01T00:00:00";
    memcpy(ref, iso8601dateTime, min(strlen(ref), strlen(iso8601dateTime)));
  
    yOff = conv2d(ref + 2);
    m = conv2d(ref + 5);
    d = conv2d(ref + 8);
    hh = conv2d(ref + 11);
    mm = conv2d(ref + 14);
    ss = conv2d(ref + 17);
  }

(RTClib.cpp, line 369)

Other definitions (https://en.wikipedia.org/wiki/ISO_8601#Times) like times only, e.g. T18:00, do not work. It is necessary to always define a full date with year, month and day before a time can be specified. Further, the colon between hours and minutes is mandatory for the constructor to work correctly. This is not the case in the ISO standard.
@edgar-bonet edgar-bonet mentioned this issue Feb 9, 2023
Open
@edgar-bonet
Copy link
Contributor

I just submitted a pull request for clarifying the documentation.

Regarding the example "T18:00", note that it would not make much sense in this context, because:

  • the other fields would have to default to something, as a DateTime object always represents a fully specified date/time
  • the only sensible default date would be the current date
  • the library has no notion of “current date”.

@matdon
Copy link
Author

matdon commented Feb 18, 2023

Dear edgar-bonet, thank you for your (incredibly) fast answer and your work in the pull request.

From my point of view, this is of great help to prevent misunderstanding in the future.
As you mentioned, it does not make sense to use a DateTime class object as defined in the library for anything else than a full date at the moment. Storing only time values is not meaningful.

However, this might not be clear at first glance when looking at the current documentation, as a DateTime objects defaults to the reference:

char ref[] = "2000-01-01T00:00:00";

and a user (as me) would come to the idea of living with the default year, month and day and storing only time information.

Your pull request helps here.

From my point of view, the issue is closed.

However one might think in the future of allowing all ISO 8601 specifiers, and point the user to the fact that default values for not specified items are used as in the current ref string. The user then can decide on how to treat the defaults on his own.
This should be mentioned in the documentation then as well.

Best regards, Matthias

@matdon matdon closed this as completed Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Clarify the documentation of the DateTime constructor from ISO 8601 edgar-bonet/RTClib
2 participants
@edgar-bonet @matdon