Add location as required argument for Time.parse

[straight-shoota]

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)

[jhass]

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]

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]

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]

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]

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]

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]

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]

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.