-
Notifications
You must be signed in to change notification settings - Fork 80
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.
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
Take fix from r-dbi to allow correct retrieval of datetimeoffset #272
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the patch.
It looks good, just an indentation nitpick.
Could you also add to the PR description references to related r-dbi PR(s) ?
Is it this one r-dbi/odbc#219 perhaps?
auto t = result.get<nanodbc::string>(0); | ||
// the result is this NANODBC_TEXT("2006-12-30 13:45:12.3450000 -08:00"); | ||
REQUIRE(t.size() >= 27); // frac of seconds is server and system dependend | ||
REQUIRE(t.substr(0, 23) == NANODBC_TEXT("2006-12-30 13:45:12.345")); | ||
auto it = t.rbegin(); | ||
REQUIRE(*it++ == '0'); | ||
REQUIRE(*it++ == '0'); | ||
REQUIRE(*it++ == ':'); | ||
REQUIRE(*it++ == '8'); | ||
REQUIRE(*it++ == '0'); | ||
REQUIRE(*it++ == '-'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation here needs some correction here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, looks like a mix of tabs and spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, the ling CI job seems fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, linter still passing after rebase. Weird.
Updated the style by running |
This PR will change the API behaviour of datetimeoffset.
It is no longer possible to get those columns via the timestamp class.
Since the timestamp class has no notion of UTC offset, allowing the datetimeoffset as a unparsed strings seems more reasonable.