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

Add custom time formats #5084

Closed

Conversation

straight-shoota
Copy link
Member

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 reorganizes Time::Formatter and introduces custom format definitions for some well-used time formats.

This accompanies #4729 which improves the external API using fixed formats.

#
# HTTP::HTTP_DATE.format(Time.new(2016, 2, 15)) # => "Sun, 14 Feb 2016 21:00:00 GMT"
# ```
module HTTP::HTTP_DATE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why moving this out of time related context into HTTP? This module still deals with date parsing and clearly states it follows RFC 2616, so why not have it inside Time like the others RFCs?

@straight-shoota
Copy link
Member Author

@luislavena The reasoning behind HTTP::HTTP_DATE was that this is a very specific combination of certain formats which doesn't have a real usecase outside HTTP protocols. Please see #4729 (issuecomment) ff.
In this PR the discussion was about the accessor methods HTTP.http_date and HTTP.parse_date. We could still keep these and move the format to Time::Format::HTTP_DATE.

I think itmakes sense to keep all format descriptions in one place.

@luislavena
Copy link
Contributor

luislavena commented Oct 6, 2017

I think it makes sense to keep all format descriptions in one place.

I will have no problem with HTTP.http_date and HTTP.parse_date become small shims/wrappers that access the specific Time::Format 😄

Definitely will make things more cleaner and easier to follow if all time related formats lives in the same space.

Thank you for taking the time to respond my comment and your continuous contributions to Crystal!
❤️ ❤️ ❤️

@asterite
Copy link
Member

Strong 👎 from me. This introduces a lot of new types, destroys the existing code, and I don't understand the purpose. If RFC_3339 needs a custom parser, write a custom parser (we can add it to the std), but I can't see why the whole Time formatting/parsing code has to change.

If it's just to reuse a bunch of small parsing methods (like, parse a day or a month) then I'd like to apply one of Go's proverbs here:

A little copying is better than a little dependency

It's better to duplicate a bit of code here. Otherwise I can't understand the code anymore with so much dependency, changes and indirections.

@straight-shoota
Copy link
Member Author

@asterite Thanks for the review. I don't like this complicated structure any more than you do.

The problem is: Duplicating all this parser and formatter code doesn't feel like a good solution either. There are not just small independent parsing methods but some are quite complex through combinations. And many are used by different parsers. So we'd really have to copy lots of code.

But I think I'll try to do it in a less invasive and more duplicating way and then we can see what can perhaps be improved.

@asterite
Copy link
Member

asterite commented Oct 14, 2017

Also, maybe this doesn't need to exist in the standard library. People can create shards for parsing specific "dynamic" time formats.

@straight-shoota
Copy link
Member Author

Well these are really very common formats that should be fully supported by stdlib. The ISO 8601/RFC 3339 format is the most common format and in stdlib used by the JSON serializer. "Http date" is used for HTTP cookies. They are currently implemented only with limited, pattern-based support but it should be standard-compliant. This can only be achieved by a custom parser, which was as I understand it, consensus in #4729.

@asterite
Copy link
Member

Golang has a huge standard library and these formats are not supported.

@straight-shoota
Copy link
Member Author

Yes they are: time.Parse and time Constants. They have also a few more which I think we shouldn't need to support.

@asterite
Copy link
Member

So let's add those constants too, and problem solved. Just note that those constants have a fixed format, not a dynamic one like the ones you are proposing.

And that's what I'm saying: if you want a dynamic format, it can be provided by an external shard, or if the std needs it for something (like for parsing a specific YAML time format, or HTTP date) we can implement those specific parsers. But there's no need to make everything more complex and generic for that.

@RX14
Copy link
Contributor

RX14 commented Oct 14, 2017

I feel uneasy merging something that doesn't cover 100% of the spec just because it works for 99% of applications.

@asterite
Copy link
Member

I know. I wouldn't like adding those constants like it's done in Go. But I also don't want to maintain this and this is one of those things that can be just fine in an external shard.

In fact, I've been thinking that maybe it would be a good idea to move XML, YAML, maybe also Big to external shards so they can evolve faster and independent of the compiler. Maybe also JSON,, except that the compiler uses that.

@straight-shoota
Copy link
Member Author

I found a much easier solution which neither creates any more types nor duplicated code (except a few boilerplate methods) by just reopening the existing classes. #5123

@straight-shoota straight-shoota deleted the jm-time-formats branch September 11, 2018 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants