-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add custom time format implementations #5123
Add custom time format implementations #5123
Conversation
src/time/format/formatter.cr
Outdated
when Time::Kind::Local | ||
time_zone_rfc2822 | ||
else | ||
raise "invalid timezone" |
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.
Capitalize exception message, please.
ef5042d
to
216efdb
Compare
92e737f
to
be405fb
Compare
I've rebased this PR and refactored a bit. It would be great to get some feedback. |
be405fb
to
a98da2a
Compare
a98da2a
to
575f4eb
Compare
18fde5a
to
1e1b76d
Compare
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.
-
may I ask to rebase this PR on master?
-
if Ensure time parser raises if timezone is missing #5382 is merged we are mostly ok since we will have proper timezone parsing exception but adds a quick-hack, and it would be more proper to include this PR instead of Ensure time parser raises if timezone is missing #5382. Am I right?
-
Is there anything pending that you have in mind as a follow up for this PR?
These adds some breaking-changes so having a rebase would be helpful to check the impact.
spec/std/http/http_spec.cr
Outdated
it "without time zone" do | ||
time = Time.utc(1994, 11, 6, 8, 49, 37, nanosecond: 0) | ||
HTTP.rfc1123_date(time).should eq("Sun, 06 Nov 1994 08:49:37 GMT") | ||
HTTP.format_time(time).should eq("Sun, 6 Nov 1994 08:49:37 GMT") |
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 preferred format is with two day digits as of https://tools.ietf.org/html/rfc7231#section-7.1.1.1 and https://tools.ietf.org/html/rfc2616#page-21
Somehow I would prefer rfc/iso in the function names for the format.
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 name was changed because the methods doesn't implement one exact standard format but generally format and parse time instances for use in a HTTP protocol context (see previous comments in #4729 (comment)).
I'll look into the preferred digit format.
src/http/common.cr
Outdated
end | ||
|
||
# DEPRECATED: Use `HTTP.format_time` instead. | ||
def self.rfc1123_time(time : Time) : String |
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.
Why this is left and marked as deprecated and rfc1123_date dropped completely? I think is either both or none.
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.
Yeah, it needs to be removed.
src/json/to_json.cr
Outdated
@@ -124,7 +124,7 @@ end | |||
|
|||
struct Time | |||
def to_json(json : JSON::Builder) | |||
json.string(Time::Format::ISO_8601_DATE_TIME.format(self)) | |||
json.string(Time::Format::RFC_3339.format(self)) |
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.
Why this change if ISO_8601 is still defined?
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 difference is mostly negligible, but RFC 3339 is generally the better (stricter) format for this purpose. ISO_8601_DATE_TIME
by default formats a offset as HHmm
while RFC 3339 requires a colon: HH:mm
The JSON specification doesn't include a Date data type, so it's up to the implementation. JavaScript's Date.toJSON()
actually is RFC 3339 compliant (YYYY-MM-DDTHH:mm:ss.sssZ
).
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 references I found said ISO8601 is the format used in Javascript. I think we should stick to 8601 here. Actually been less stricter is better IMO since an API built on top of this would be less pedantic to the user.
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.
True, this is a bit confusing because RFC 3339 actually is ISO 8601. But RFC 3339 is better suited as machine-parsed exchange format. That's why I'm pretty convinced we should use RFC 3339 as output format for #to_json
.
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.
I left RFC_3339
here but it returns the exact same result as ISO_8601_DATE_TIME
.
1e1b76d
to
81cc01f
Compare
Thanks for taking a look at this!
|
spec/std/json/serialization_spec.cr
Outdated
@@ -169,7 +169,7 @@ describe "JSON serialization" do | |||
end | |||
|
|||
it "deserializes Time" do | |||
Time.from_json(%("2016-11-16T09:55:48-0300")).to_utc.should eq(Time.utc(2016, 11, 16, 12, 55, 48)) | |||
Time.from_json(%("2016-11-16T09:55:48-03:00")).to_utc.should eq(Time.utc(2016, 11, 16, 12, 55, 48)) |
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.
We shouldn't break this
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.
According to #5123 (comment)
I agree. The JSON format should be ISO8601 by default.
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.
Output should be the RFC 3339 flavour of ISO 8601, the parser should accept more variants. That's probably the best solution.
After 2 is addressed is a 👍 from me (and a big 🙇 for all the time re work) |
|
If you prefer me to made the changes in 2 let me know. |
Done. To accomplish this, I added a |
e569afc
to
cb2e4fa
Compare
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.
I don't like that there are changes to time/format/formatter.cr
and time/format/parser.cr
, and I don't like that there are new methods just moneypatched in to the existing parser and formatter, instead of being inherited or stuff being put in a module.
# * **%3N**, **%L**: milliseconds, zero padded (000, 001, ..., 999) | ||
# * **%6N**: microseconds, zero padded (000000, 000001, ..., 999999) | ||
# * **%9N**: nanoseconds, zero padded (000000000, 000000001, ..., 999999999) | ||
# * **%N**: second fraction, zero padded. (Same as `%9N` but may consume more than 9 digits while parsing) |
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.
Why this doc change?
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.
It helps discoverability if the directives are lexically ordered. It's unrelated, though. If you like, I can make a separate PR for this.
@@ -23,6 +23,10 @@ struct Time::Format | |||
io << time.year / 100 | |||
end | |||
|
|||
def full_or_short_year | |||
year |
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.
Why an alias?
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 parser has a different implementation.
@@ -79,6 +83,12 @@ struct Time::Format | |||
io << get_short_day_name.upcase | |||
end | |||
|
|||
def short_day_name_with_comma? |
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.
This is a duplicate definition.
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.
Where's the duplicate?
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.
@RX14 You mean more like #5084? That design has been abandoned for good, I hope. This is the best I can think of and I don't see any problems with monkey patching a few specific methods nor any benefits in extracting them to modules. |
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.
I still don't get why time/format/formatter.cr
and time/format/parser.cr
are changed. If these still have the same functionality, why do they suddenly have extra unused methods? If you're going tor refactor core time formatter stuff, it should at the very least be in a seperate commit.
# * **%m**: month number, zero padded (01, 02, ..., 12) | ||
# * **%_m**: month number, blank padded (" 1", " 2", ..., "12") | ||
# * **%-m**: month number (1, 2, ..., 12) | ||
# * **%M**: minute, zero padded (00, 01, 02, ..., 59) | ||
# * **%3N**, **%L**: milliseconds, zero padded (000, 001, ..., 999) |
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.
duplicate definition of L
They provide additional methods that are used by several specific formats. So there is added functionality for the core formatter/parser feature. Refactoring is actually in 0c242b0. |
Needs a rebase though |
cb2e4fa
to
fee9ec4
Compare
@straight-shoota I rebase the changes already. |
The format patterns from Time::Format allow to define simple date formats but they are quite limited in flexibility. Many standardized formats include slight variations which must be taken into account for parsing such formats in a standards-compliant way.
This PR adds custom format implementations for some well-used time formats.
For a few it adds adds short-cut methods to
Time
class (likeTime.parse_iso8601
).Time::Format::ISO_8601_DATETIME
: Parser for standard time format with many variations. It should support most of the standard format including week dates, ordinal dates and optional separators. Uses RFC 3339 as formatter.Time::Format::RFC_3339
: More strict variation of ISO 8601, recommended for exchanging time data in computer systems.Time::Format::RFC_2822
: Format compatible to RFC 822 and RFC 1123 used for example in HTTP protocols.Time::Format::HTTP_DATE
: Proper implementation ofHTTP.parse_date
supporting time formats expressed as RFC 1123/RFC 2822, RFC 850 or asctime which are used by many HTTP protocols. Uses RFC_2822 as formatter.Time::Format::YAML_DATE
: Refactored implementation of parser for YAMLdatetime
type.Surpasses #5084, #4729
/cc @asterite @RX14