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.

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

Change RFC3339_RE to accept offset like +08:00,+0800 or +08/+8 #1167

Closed
wants to merge 2 commits into from
Closed

Conversation

guo-steve
Copy link

@guo-steve guo-steve commented Dec 15, 2017

Summary

Make Mojo::Date->parse support ISO 8601. Almost all date formats are supported, such as '2014-08-20 20:45:00+00', '2014-W34-3T20:45:00', '2014-232T20:45:00'.

Motivation

Although RFC3339 is commonly used on the internet, we could still found other ISO 8601 date time format widely used in other areas, e.g. PostgreSQL. So, the small changes would make Mojo::Date more useful in web application development.

@jhthorsen
Copy link
Member

I think this change makes sense, but you need to provide unit tests for the PR to be accepted.

@kraih
Copy link
Member

kraih commented Dec 15, 2017

What would be the documentation situation for this change? How do we explain the partial ISO 8601 support (outside of RFC 3339) to our users?

@kraih
Copy link
Member

kraih commented Dec 18, 2017

Afraid i have to give this a 👎, since the documentation does not convince me. Specifically i'm not convinced that partial ISO 8601 support adds enough value to make up for the possible confusion it may cause.

@guo-steve
Copy link
Author

So, a complete ISO 8601 support would be better or completely ISO 8601 is not considered?

@kraih
Copy link
Member

kraih commented Dec 23, 2017

What would we want full ISO 8601 support for?

@kraih
Copy link
Member

kraih commented Dec 23, 2017

Adding RFC 3339 support was an easy decision, since it is used in many web services. What would we use ISO 8601 for?

@jberger
Copy link
Member

jberger commented Dec 23, 2017

It would save me from having to fall back on Time::Piece in Mojo::XMLRPC https://github.com/jberger/Mojo-XMLRPC/blob/abf27ee6bd7f2cb842fb3cde22e7c7347a232d66/lib/Mojo/XMLRPC.pm#L179-L185

@kraih
Copy link
Member

kraih commented Dec 23, 2017

If we decide that ISO 8601 is a reasonable addition to Mojo::Date, the implementation will need a formal spec review, to make sure it is compliant and covers the whole spec.

lib/Mojo/Date.pm Outdated
$month = 0;
unless($day) {
my $days = 0;
$days += Time::Local::_is_leap_year($_)?366:365 for (1970..($year - 1));
Copy link
Member

@kraih kraih Dec 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function _is_leap_year() does not appear to be part of the public Time::Local API.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a private method of Time::Local. It is very simple so it can be copied to Mojo::Date if possible.

@kraih
Copy link
Member

kraih commented Dec 23, 2017

And if RFC 3339 is a profile of ISO 8601, why is there still separate RFC 3339 code?

@mohawk2
Copy link
Contributor

mohawk2 commented Dec 23, 2017

I would like to see:

  • support for ISO 8601!
  • full spec compliance
  • better test messages than "right epoch value": say which feature is being tested on each one
  • why not incorporate the 3339 stuff into this, if @kraih is right
  • documentation that is up to usual Mojo standards

@jberger
Copy link
Member

jberger commented Dec 23, 2017

I should clarify, I think supporting ISO 8601 would be a nice thing. I have not reviewed the proposed patch having been too busy with the advent calendar.

@guo-steve
Copy link
Author

My initial thought for adding ISO 8601 support is just to make Mojo::Date to support date time format of TIMESTAMP in PostgreSQL. So, I could probably use Mojo:Date to replace DateTime. Well, I would also want to add some more features like date time comparison, addition, subtraction etc. Although, I am not sure whether Mojo::Date is designed for having these features.

@kraih
Copy link
Member

kraih commented Dec 25, 2017

Mojo::Date was not designed for having those features, it's primarily meant for dealing with HTTP header fields. And i don't think PostgreSQL is a good reason to add ISO 8601 support, since it has such good tools already for dealing with timestamps, like extract(epoch from foo) as foo.

@guo-steve
Copy link
Author

guo-steve commented Dec 25, 2017

Understand now. So maybe I extend the class for my own use. :D.

@guo-steve
Copy link
Author

guo-steve commented Dec 25, 2017 via email

@kraih
Copy link
Member

kraih commented Feb 1, 2018

I'm not sure how to move forward with this pull request. At the moment it seems out of scope for Mojolicious to have full ISO 8601 support. Are there any real world web development use cases? Any APIs or protocols using the format?

@dotandimet
Copy link
Contributor

I'd be happy if Mojo::Date could be used to parse Atom/RSS dates, and ISO 8601 support seems like a step in that direction.

@kraih
Copy link
Member

kraih commented Feb 4, 2018

@dotandimet Don't those use RFC 3339 and RFC 822? Both are already supported by Mojo::Date.

@kraih
Copy link
Member

kraih commented Feb 4, 2018

More people have brought up the argument that ISO 8601 support would be good for working with PostgreSQL. And i'm afraid that argument did not convince me, in fact it did the opposite. I now believe that it would just encourage bad practices, like avoiding the use of more efficient native PostgreSQL features such as extract(epoch from foo) as foo.

We've tried very hard to find other use cases on IRC, but none could be found, so i'll have to vote 👎 on this proposal for now.

@dotandimet
Copy link
Contributor

Oh, I see that RFC3339 (what the Atom spec uses) is a subset of ISO 8601.
Of course, what Atom/RSS feeds in the wild use is a different thing (DateTime::Format::RSS just throws parsers at the string until something sticks).
Off-topic, I now want Mojo::Date to support timezones (perhaps by using Time::Zone, if installed?)

@kraih
Copy link
Member

kraih commented Feb 4, 2018

@dotandimet Please stay on topic, we are voting only on this patch.

@jberger
Copy link
Member

jberger commented Feb 4, 2018

As I commented before, XMLRPC spec designates ISO8601also. That said I think that the response to this proposal seems to suggest there's some public interest in supporting more formats but not strong in use cases. Therefore I wonder if we should encourage roles for adding more formats.

Given that perspective I give this proposal a weak 👎

@kraih
Copy link
Member

kraih commented Feb 4, 2018

Since there are no core uses for the format, it could most definitely be a role.

@marcusramberg
Copy link
Member

I would like to see more powerful date support in Mojo::Date, but I think a role is the way to go. 👎 to adding this to core, but would love to see it on CPAN.

@kraih
Copy link
Member

kraih commented Feb 5, 2018

Ok, i think we can conclude that this proposal did not reach the required votes. The primary reason being lack of real world use cases. Therefore this vote only means that we won't merge the patch at this point in time, but will not rule out a new vote in the future.

It seems likely that full ISO 8601 support could become relevant again with new developments, the web is a moving target after all. Until then i think we all agree that it would be great to see this feature turned into a role (Mojo::Date::Role::ISO8601?). Thank you @guo-steve for this proposal.

@kraih kraih closed this Feb 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants