-
Notifications
You must be signed in to change notification settings - Fork 576
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
Conversation
I think this change makes sense, but you need to provide unit tests for the PR to be accepted. |
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? |
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. |
So, a complete ISO 8601 support would be better or completely ISO 8601 is not considered? |
What would we want full ISO 8601 support for? |
Adding RFC 3339 support was an easy decision, since it is used in many web services. What would we use ISO 8601 for? |
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 |
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)); |
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 function _is_leap_year()
does not appear to be part of the public Time::Local API.
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 is a private method of Time::Local. It is very simple so it can be copied to Mojo::Date if possible.
And if RFC 3339 is a profile of ISO 8601, why is there still separate RFC 3339 code? |
I would like to see:
|
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. |
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. |
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 |
Understand now. So maybe I extend the class for my own use. :D. |
Anyway, after learned what extract(epoch from foo) as foo is, I don't
really think that's a good practice for me. It looks I will always have
to do that trick when I retrieve timestamp from the database. :-)
…On Mon, Dec 25, 2017 at 9:30 PM, Sebastian Riedel ***@***.***> wrote:
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.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1167 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHeLbdKW2q1vY4Yl5_D1F7L2PzgpH9qMks5tD6NzgaJpZM4RDFEy>
.
--
*Steve Guo*
|
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? |
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. |
@dotandimet Don't those use RFC 3339 and RFC 822? Both are already supported by |
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 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. |
Oh, I see that RFC3339 (what the Atom spec uses) is a subset of ISO 8601. |
@dotandimet Please stay on topic, we are voting only on this patch. |
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 👎 |
Since there are no core uses for the format, it could most definitely be a role. |
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. |
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 ( |
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.