-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix time parse doc #6258
fix time parse doc #6258
Conversation
The output is actually |
I did not verify it. 😞 |
7801179
to
31f3c89
Compare
Using the local time zone is not always a sane default and can lead to unexpected errors. |
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 exact output of the zone offset depends on the local time zone, so it will inevitably vary. It would probably be better to just use a specific time zone for this.
31f3c89
to
88aecdb
Compare
Ping, could it merge? |
I don't understand this api. Location is optional, but if a location can't be determined it raises? Like, imagine the time comes from user input: sometimes it works, sometimes it raises. I think the default should be UTC, or else require a location, and maybe have Just an opinion, though, but I think every other language assumes a location. |
Assuming any location (be it UTC, local or anything else) is error prone and such an errors would be hard to detect. But it could be an improvement to always require a value for It could just be like this: def self.parse(time : String, pattern : String, location : Location) : Time
Format.new(pattern, location).parse(time)
end
def self.parse_utc(time : String, pattern : String) : Time
Format.new(pattern, Location::UTC).parse(time)
end
def self.parse_local(time : String, pattern : String) : Time
Format.new(pattern, Location.local).parse(time)
end
def self.parse_with_location(time : String, pattern : String) : Time
Format.new(pattern, nil).parse(time)
end |
This can be closed due to #6369. The fix was included there. |
Fixed document but i still ask why not use
Time::Location.local
by default if time string was not contain timezone?