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

Rename Time constructors #5346

Merged
merged 4 commits into from Feb 12, 2019

Conversation

Projects
None yet
8 participants
@straight-shoota
Copy link
Member

commented Dec 4, 2017

This is a follow-up to #5321 which added the Time.utc constructor.

I think this constructor should allso be callable without arguments resembling Time.new with location UTC. You can use Time.new() for the current and Time.new(2017, 12, 4, 12, 51) for a specific time instance in local time. Time.utc(2017, 12, 4, 12, 51) and Time.utc() should be usable in the same way to get time instances in UTC.

There is already Time.utc_now which could stick around as an alias like Time.now for Time.new or we could remove it.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-utc_now branch from f3f8177 to 34d08c1 Dec 4, 2017

@RX14

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

I think we should go the other way, remove the new with args. We'd end up with 2 constructor groups:
.new which take a specific moment in time as arguments, and now and utc_now which construct a time instance by consulting the system clock.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2017

This would be equally consistent as my suggestion, but I'm not sure it would be wise to go this separate way. I believe it makes the API more complicated without a real benefit.
To me, it seems natural that calling a constructor of Time without any arguments returns an instance holding the default value, i.e. the current time. That's the way of the least surprise.

If we'd want to get rid off the alias, I'd rather lose Time.now - it's an additional method to remember and Time.new() comes just natural. This is even more true for the UTC variant: People shouldn't have to remember if it was Time.utc_now or Time.now_utc. Using just Time.utc is easier, shorter and equally expressive.

@RX14

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

We should have Time.now take a timezone argument after #5324. It will return Time.now as if the current timezone was the same as the argument you pass in.

Then it's simple: Time.new for specifying a point in time which isn't now, Time.now for specifying now (just in a few seperate encodings).

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2017

Why force two names when one would be enough? We can have both Time.new(location = Location.local) and Time.new(year, month, day, hour, minute, second, location = Loation.loal) (that's already in #5324) and in the same manner Time.utc() and Time.utc(year, month, day, hour, minute, second). I'd take that for the smartest API.

@RX14

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

On the flipside, why not have 2 names form doing 2 different things. Technically, both solutions are equal. I prefer to distinguish the two cases by name to make it clear what Time.new returns (now).

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

Time.utc_now always felt too long to me. I'll welcome renaming it to Time.utc.

Still on personal preference, I'd go for dropping the argless Time.new which duplicates Time.now.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2017

Any other ideas about this? @RX14 what do you think about @ysbaddaden's suggestion to rename utc_now to utc which IMHO is a clear win because it is shorter and equally expressive? That's exactly what this PR is supposed to do (should utc_now be removed entirely or deprecated?). We could discuss new vs. now separately? Though I'm not sure if it would be benefitial to split off this discussion...

@bew

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

I don't think utc & utc_now are equally expressive by the name.. Time.utc doesn't sound like you're trying to get the current time to me.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2017

What else would it mean? Maybe I'm just misguided by my past experience, but as far as I can remember, it is common practice in object oriented programming that if you create a time instance without any arguments it is initialized to represent the time at which it was created. For example Ruby Time.new, Java new Date(), JavaScript new Date(), PHP new DateTime(), Lua os.time() all operate in that way.
I don't see any benefit in adding now in the constructor because this should be evident when there is no value describing any specific time.

@RX14

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

I still prefer this:

now specified time
local/configurable Time.now Time.new
UTC Time.utc_now Time.utc

I'm welcome to rename these, but I'd like to keep separate names for each cell, with the UTC ones simply being aliases of the local ones with the kind pre-sepcified.

@paulcsmith

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

I was tripped up by this the other day because I thought Time.new would be utc. I think it would be clearer if we did something like this:

type now specific time
local Time.local Time.local(args)
UTC Time.utc Time.utc(args)

I could also see it being local_now and utc_now, but I like the idea of having local in the name so people know what they are getting. This could prevent a lot of timezone bugs

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2017

@paulcsmith This looks also interesting. I would certainly prefer that over having different names for current/specific instance.

But I am not sure if changing from new to local would help that much.

Generally, there is the issue that using new/local to create a specific time completely arbitrary in respect to incremental time. You can't know what the local time zone will be when this instance is created. It won't even reliably work if you're comparing different specifc time instances because the local time zone might change in between.

time_a = Time.new(2017, 12, 13, 21, 17)
# some code in between 
time_b = Time.new(2017, 12, 13, 20, 17)

If nothing is changed in between, time_a - time_b will equal 1.hour. If for example the code in between results in changing the TZ env var, or the system's time zone is changed at some point, the result can vary about several hours, it might even be negative.

A safe way would be to store the time zone for reference later:

location = Time::Location.local
time_a = Time.new(2017, 12, 13, 21, 17, location: location)
# some code in between 
time_b = Time.new(2017, 12, 13, 20, 17, location: location)

This would ensure that both times are in the same time zone and time_a - time_b equals 1.hour no matter what happens in between (apart from changing the local variable).

If you want to reference a specific point in incremental time, you can't use a constructor with implicit local location. I'm wondering if it should be supported at all... it would help avoid a lot of problems if you'd have to be explicit. If you want to reference a specific point in time, it should be expressed with an explicit time zone (preferably UTC to avoid potential ambiguity).

The "now" constructor with implicit location will always reference one specific point in incremental time. Several time instances created by Time.now can be compared without issues, it doesnt matter if the local time zone changed in between.

@paulcsmith

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

I really like the idea of always requiring a time zone. We ran into a few bugs because of this confusion. Making it explicit would have been a big help 👍

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

What do you think about the following constructors for Time:

location current calendrical timestamp
specified Time.local(location) Time.local(year, month, day, hour, minute, second, nanosecond, location) Time.local(seconds, nanoseconds, location)
local Time.local Time.local(year, month, day, hour, minute, second, nanosecond) Time.local(seconds, nanoseconds)
UTC Time.utc Time.utc(year, month, day, hour, minute, second, nanosecond) Time.utc(seconds, nanoseconds)

Besides that, there are still parser methods (parse, parse!, parse_local, parse_utc) and for UNIX timestamp (epoch, epoch_ms).

All in all, this would be pretty straightforward and easy to follow, having just two methods with different sets of arguments.

Instead of Time.local, all non-UTC methods could also be called Time.new but without location argument it is not explicit about which location it uses and this is bad as per @paulcsmith's argument. Time.local makes it clear that it constructs a local time instance.

An alternative would be to use Time.new for specific location. But I think it makes more sense to keep the same name for both and only differentiate by the presence of location argument (which default to local location).

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

Please don't merge seconds/nanoseconds timestamps with other constructors. It's confusing, and kinda internal detail.

What we want is a UNIX timestamp, easily usable for the common seconds / milliseconds form (others seem uncommon). I'm not fond of the epoch and epoch_ms namings, but at least they're somewhat descriptive —I'd use .unix I think.

I like the .local and .utc.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

The seconds/nanoseconds constructors are currently exposed as .new(seconds, nanoseconds, location) and .utc(seconds, nanoseconds) so I think it makes sense to include them into the considerations. They resemble the internal representation, but this is quite important, because it is more performant than the calendrical constructor and allows to integrate extensions to the time API very easily (additionally, Time#total_seconds should be public for this).

I'm fine to leave them as is (.new/.utc) differently from the other discusses constructors. But it might seem a little bit inconsistent.

I agree that .epoch is not optimal, especially considering that epoch of Time is actually not Unix epoch (1970-01-01) but 0001-01-01.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

While following my propsal, I noticed that throughout stdlib, very often Time.new() and Time.now() are used when they're not really necessary. In most cases it is just about retrieving a current time stamp and doesn't need any location attached. An obvious example is Time.now.epoch, which goes a long way to retrieve the current location (potentially reading from the file system) just to immediately discard it.

In my opinion this usage pattern supports renaming .new and .now to .local to make it more obvious for the developer to decide explicitly between utc and local instead of a generic new/now.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-utc_now branch 3 times, most recently from 15383e1 to a8eaad0 Sep 4, 2018

@straight-shoota straight-shoota changed the title Add arg-less Time.utc constructor Rename Time constructors Sep 4, 2018

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

The seconds/nanoseconds constructors are currently exposed as .new(s, ns, location) and .utc(s, ns)

They're internal implementation details of Time and Time::Span; they should be either protected or tagged with :nodoc:. They may be faster to use now, but that's a side effect of exposing internal details; changing the internal API can break this assumption at any time (bad) or prevent changes to the internal API (bad).

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

Let's not bother with the seconds, nanoseconds constructor here, they're not even touched in this PR. I just included them in the overview to see how they could be changed (in a different PR).

This PR is about renaming Time.now/.new to Time.local and Time.utc_now to Time.utc to improve the API making it a little bit simpler, more expressive and in total easier to understand

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-utc_now branch from a8eaad0 to ed8403f Jan 7, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Continuing from my latest proposal, which seems like everyone could agree on, I've rebased this PR on master and updated the changes.

This will lead to these methods to create generic time instances:

location current calendrical
specified Time.local(location) Time.local(year, month, day, hour, minute, second, nanosecond, location)
local Time.local Time.local(year, month, day, hour, minute, second, nanosecond)
UTC Time.utc Time.utc(year, month, day, hour, minute, second, nanosecond)

(Additionally, there are also .unix and .parse methods to import from timestamp or string representation)

I've split the changes into logical commits, each performing one of the rename/remove operations required to implement the desired schema.

I hope this gets your thumbs up 👍

@bew

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Please add the labels for this PR (especially for breaking-change!)

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-utc_now branch 2 times, most recently from 74a9c1b to 6dad63a Feb 10, 2019

straight-shoota added some commits Jan 7, 2019

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-utc_now branch from 6dad63a to 5c4fffa Feb 11, 2019

@straight-shoota straight-shoota merged commit 323ed75 into crystal-lang:master Feb 12, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@straight-shoota straight-shoota deleted the straight-shoota:jm-utc_now branch Feb 12, 2019

@asterite

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

This will be a massive breakage, but I guess it's fine. I liked Time.now but it's true that it's not clear whether that's local or UTC... that said, I always think of Time.now as local, so what was wrong with that?

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

I guess Time.now would just be an alias for Time.local 😛 Hence it adds more methods to learn and a little bit of uncertainty whether it's local time.

But that's not really a big issue and I would be totally fine with keeping (or re-adding) it as an alias.

@asterite

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Let's see what happens with this change in the next release. We are always experimenting so it's fine to try new ideas (I think it's a pretty radical change, but the idea of clarity is really good).

@Sija

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

IMHO that's one of the few cases where I'd say such an alias would be desirable - there's nothing nice about breaking tons of code, or making steeper learning curve for people coming from Ruby-land.

@paulcsmith

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

One thing I love about Crystal is that there are fewer aliases than in Ruby. One thing we may want to consider is adding a helpful error when using Time.now so people know what they need to do:

def now
  {% raise "'Time.now' has been renamed to 'Time.local'" %}
end

This could also be done with the other methods that have been renamed to help people out.

This has worked well in Lucky so far because it's allowed us to keep things clean and focused but also help people find out what they need to do without looking at changelogs or asking for help. Thoughts?

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

local is a good and short name, let's deprecate nowfor a while.

@Sija

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

@paulcsmith Nice error message or not, it still breaks the code nevertheless for very lil' gain. Apart from having less aliases than Ruby what I personally like in Crystal is striving for a balance between ideology (no aliases) and clean APIs (3.seconds.ago). I believe in importance of moderation (See Goldilocks principle), which is at stake when ideology is over-represented. Time.now intuitively implies locality, since when referring to now in most cases you'd think here and not now in Alaska or now in GMT but maybe that's just me...

@paulcsmith

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

I actually don't have a strong preference either way for now or local and I also love the idea of Goldilocks principle. But in general, fewer aliases is good IMO. And if we do remove now then I think having a good error message would be great.

FWIW I also think certain breakages are not a big deal. In this case all you need to do is a find and replace across your project and you're done. But really this suggestion is more about how to handle deprecations and removal to make things as easy as possible. In the past stuff has been renamed/moved and the only way to know was to check the changelog. Adding a nice error message at compile time would make it a lot easier to make changes to Crystal w/o causing too much trouble

@asterite

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

My preferred way is to deprecate functionality with a compile time warning in one version, then remove it in a next version. That way you can upgrade, your code still compiles (and dependencies too), and eventually by the next version everything's working fine. The point about dependencies is really important.

@paulcsmith

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Totally agree 👍 But even after the deprecation I think having the compile time error is nice for those that are used to the Ruby method names for example. Or that have been out of the Crystal ecosystem for a bit and are coming back after a few months

@asterite

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@paulcsmith You mean, like, forever?

@paulcsmith

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Depends on the method I think. If it is something that people might expect coming from another language, or that they often get wrong, then yeah I'd leave it forever because it makes using Crystal really nice and help the user get where they want to go. If it is something just in Crystal that has changed (like when Server#listen changed) then I'm not sure. Maybe ~3-6 months after deprecation?

So for example. If we do keep Time.local and remove Time.now then yeah I'd leave some kind of compile time message error for Time.now like Please use 'Time.local' or something`

@Sija

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Sorry, I can't help to think wouldn't be better to make Time.now an alias, instead of breaking peoples expectations and littering the codebase with compile-time warnings? I mean, that's one alias, and the one many people know/are used to, why to over-complicate things?

@paulcsmith

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Only at the beginning you would have compile-time warning. Then you (the library author) change the method calls and they're gone. After that they raise errors (so new code would get an error and you'd fix it immediately). No littering with warnings. I think this is better than an alias for a few reasons:

  • You don't need to know two methods.
  • You don't need to wonder why a codebase sometimes uses Time.now and sometimes Time.local. Are they the same thing? Do they work slightly differently like ActiveRecord.size/count/length?

By having just one method you don't need to wonder why someone is using one or not the other. You never need to look it up.

So I'd flip the question around. Why would you want an alias? Once everything is converted to the new method there is no long term benefit to having an alias. The benefit is only short term IMO. It makes upgrading Crystal easier.

I'd say right now Crystal is still in its early stages and we should not be adding aliases yet unless we have a really good reason. Maybe if it were more mature it would be better to keep the alias around, but right now I think really nailing the API is key even if it means a bit of extra work for maintainers right now. Having a cleaner long term solution is a better tradeoff IMO

@Sija

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

@paulcsmith These are good arguments, I'm convinced by lack of long-term benefits—other than familiarity to Ruby devs. In the short term it'll be a PITA and many under/not-maintained shards will suffer from that but oh well...

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

and many under/not-maintained shards will suffer from that

They're gonna break sooner or later anyway, so it's just making it break earlier. Which might even be a good thing because it makes people notice it's unmaintained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.