CBL-5438: DateTime standard format parser #213
Conversation
|
See Core PR couchbase/couchbase-lite-core#1977 |
|
First issue: Unsure whether CBL3.1 included milliseconds by default or not. I thought it didn't, so I removed milliseconds from the default format (found at |
|
Are there tests for this somewhere? Could they be added to the Fleece tests? Also I'm kind of unsure if this qualifies as feature creep or not since this is getting out of the scope of a binary encoding. @snej Do you have an opinion on this? This functionality needs to be somewhere accessible to Fleece obviously but is it ok to live next to fleece itself? |
|
I added a couple tests in Core. Probably does need more. And I'm not sure. Previously we parse date format strings as DateTime, which restricts us severely. For example, we must use no-colon by default in Timezone specifier, but using DateTime as format we can't offer colon delimited Timezone. So as far as the parser goes, I don't see that the scope is any bigger than it already was (although it may have already had scope creep). However, the %-format is not strictly necessary (although much nicer). |
|
Can those Core tests be moved here in some fashion and use the underlying classes directly? |
|
That would be a good idea... Having to run a Core PR pointing here to make sure nothing's broken over here isn't the best way to write software.... |
Some DateFormat fixes.
|
@borrrden I've added some DateFormat tests. See |
| static slice format(char buf[], int64_t timestamp, std::chrono::minutes tzoffset, | ||
| std::optional<DateFormat> fmt); | ||
|
|
||
| static DateFormat kISO8601; |
|
|
||
| bool operator==(const YMD& other) const; | ||
|
|
||
| static YMD kISO8601; |
| }; | ||
|
|
||
| // 1111-11-11T11:11:11(Z) | ||
| DateFormat(YMD ymd, Separator separator, HMS hms, const std::optional<Timezone> tz = {}) |
There was a problem hiding this comment.
The const here doesn't do anything, since tz is not a pointer or reference
There was a problem hiding this comment.
But, it is essentially a variable declaration. Which (IMO) should be const if it isn't modified in the function.
| explicit DateFormat(YMD ymd) : ymd{ymd} {} | ||
|
|
||
| // 11:11:11(Z) | ||
| explicit DateFormat(HMS hms, const std::optional<Timezone> tz = {}) : hms{hms}, tz{tz} {} |
There was a problem hiding this comment.
Another const that doesn't do anything
| // %F == %Y-%m-%d | ||
| if ( formatString[1] == 'F' ) { | ||
| // ISO YMD, so we don't need to change `ymd` | ||
| formatString.setStart(formatString.begin() + 2); |
There was a problem hiding this comment.
slice_istream would be pretty useful here, making the code simpler & safer
| enum class Timezone : uint8_t { NoColon, Colon }; | ||
| enum class Separator : char { Space = ' ', T = 'T' }; | ||
|
|
||
| struct YMD { |
There was a problem hiding this comment.
It looks as though the only parts of YMD and HMS that are used by this PR are the separators. I understand wanting room to grow, but all the work of defining types and fields and structs that aren't used yet seems too much ... It seems like each of these structs could be reduced to the Separator enum for now, and it wouldn't be difficult to refactor them back to structs in the future if/when we need it.
There was a problem hiding this comment.
Yeah... I laid out the types before I'd written the functionality, and before I realized how restrictive we have to be. It started out with a decent number of variants in a few of these types, but they were removed as I progressed along building up the format we actually allow. I've stripped these out now, in the Core PR.
|
I believe the counterpoint to "this is only used in LiteCore" is "we were using some of this for a specific function in here which writes a datetime to a stream". In that case at least some of this needs to be accessible here which makes breaking it up somewhat of a pain. |
|
Closing. Moved |
|
@borrrden Regarding
For now I'm just going along with the duplicated code. Jens said he expected some duplicated code around this, and would rather this not be in Fleece. I definitely agree it doesn't really belong in Fleece, but I think the only solution which doesn't involve duplicated code would be adding a whole new dependency. Which I feel would be more pain than it's worth. It would involve ripping out basically all Date-related functionality from Fleece, and requiring it depend on this dependency. |
A proper parser for DateTime formats, rather than just parsing format string as a DateTime.
This has benefits such as additional flexibility in future, but also allows us to keep consistent with the strange "almost-ISO" date format we shipped with CBL 3.1, because keeping format as a
DateTimedoes not allow us to optionally use colon or no-colon for Timezone offset (i.e.+0500/+05:00).I added in parsing for a "subset" of https://howardhinnant.github.io/date/date.html#to_stream_formatting, but it's not necessary and if anyone thinks we don't need it, happy to take it out. Think it's a lot better than the weird "1111-11-11T11:11......` format we copied from Server though.