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 location as required argument for Time.parse #6369

Merged

Conversation

straight-shoota
Copy link
Member

This makes it more clear in the API that a default value is required instead of defaulting to nil which would raise in case of time zone information missing from the formatted string. Time.parse_utc and Time.parse_local are added as short cuts for the most common use cases.

See #6258 (comment)

This makes it more clear in the API that a default value is required instead of defaulting to `nil` which would raise in case of time zone information missing from the formatted string. `Time.parse_utc` and `Time.parse_local` are added as short cuts to the most common use cases.
@jhass
Copy link
Member

jhass commented Jul 12, 2018

The convenience overloads are good, but I'm not sure I like either the current API or the proposed ones too much. nil just seems like the wrong signal value for a user facing API here and parse_with_location is not a great name. Can we maybe do

def parse(time : String, pattern : String, location : Location)
def parse(time : String, pattern : String, *, raise_on_missing_location : Bool)

?

Honestly personally I would be fine with

def parse(time : String, pattern : String, location : Location)
def parse(time : String, pattern : String) # raises, separate overload for nicer docs and nobody doing a meaningless (..., nil)

Yes, it's a runtime error but it's rather uncommon to parse a diverse set of time formats at the same time, so caught early in development.

@straight-shoota
Copy link
Member Author

straight-shoota commented Jul 12, 2018

You second suggestion is essentially the same as we have currently:

def parse(time : String, pattern : String, location : Location? = nil)

The issue with this solution is that the shortest overload parse(String, String) is the least safest. Requiring an explicit argument (or different name) makes this less dangerous. nil as an argument isn't very explicit about what it does, though. But it fits quite nice logically (if location is nil, you can't create a Time instance with it). However, it can easily lead to unintended beaviour if the value of location comes from a nilable variable.

So, maybe even more explicitness would be nice. This could either be a required argument (parse_with_location), a different name (parse_with_location, or maybe parse!??) or even a different type (for example location: :raise instead of location: nil).

@jhass
Copy link
Member

jhass commented Jul 12, 2018

You second suggestion is essentially the same as we have currently

Hence my first suggestion first and my personal opinion second. As I wrote in the second suggestion the separate overloads would be for nicer docs and code completion/IDE integration. I do see your point though and hence my compromise proposal stands first. Though I see now it doesn't make sense since there's no sensible behavior for passing false.

How terrible would it be to have a fake location like Location::Unknown or Location::Raise?

@asterite
Copy link
Member

Alternatively, make the current parse assume a location. Go for example assumes UTC.

But I'm not sure. In my workplace, for the current project, we have a tool where you specify a range of dates in the command line. It's a C# application. C# also seems to assume a location, and now I'm curious which one is it. This program sends the date to a server, and should probably be in UTC, or the server's time. So forcing the user to think a bit about whether they want UTC or local by default might be a good idea, even if it's a bit annoying.

But also, maybe the current behavior isn't bad, like jhass says, because you'll find out soon if there's something wrong (unless it's user input, which might not be a common case).

I can't make up my mind about this topic :-P

@straight-shoota
Copy link
Member Author

If we had Location::Unknown, I'd expect parse not to raise but return a Time with that location. That's possible but delays the problems to later usage of this instance. It would essentially be a floating time and that's what we had with Kind::Unspecified prior to Time::Location (see #5324 (comment), #5332). It could still raise (even more so with Location::Raise) but it would dirty the API with unusable Location instances conveying special meaning. Using nil makes even more sense to me than that.

@straight-shoota
Copy link
Member Author

I tend to like using a different method name altogether. It clearly communicates a difference in behaviour and doesn't do so over special argument types or values. parse! would be great for this, the exclamation mark signals for more special attention compared to parse.

@straight-shoota
Copy link
Member Author

For reference, I just noticed a similar use case in String#encode. The method signature is encode(encoding : String, invalid : Symbol? = nil). If invalid is nil, it raises on an invalid multibyte sequence. If :skip, it will be skipped. That's essentially similar to the current Time.parse implementation.

@straight-shoota
Copy link
Member Author

I added a commit that changes parse to always require a location and parse! as a variant without location argument that eventually raises if there is no time zone information in the formatted string.

@RX14 RX14 added this to the 0.26.0 milestone Aug 1, 2018
@RX14 RX14 merged commit 1616db5 into crystal-lang:master Aug 1, 2018
@straight-shoota straight-shoota deleted the jm/feature/time-parse-location branch August 1, 2018 12:47
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

4 participants