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 deprecation notice for Time.now #7586

Merged

Conversation

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

commented Mar 25, 2019

This is a follow up on #5346 and adds the following error message for Time.now, guiding the user to fixing this breaking change:

`Time.now` has been removed in Crystal 0.28.0, use `Time.local` or `Time.utc` instead.

More information on this change: https://github.com/crystal-lang/crystal/issues/5346
@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Sigh, sometimes I wish we'd keep a few aliases. I already fathom how much time I'll lose with Time now failing to compile and have to dig to find Time.local that I'll never remember...

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

I'm not strictly opposed to make Time.now an alias. I don't think it would hurt much and improve quality of life for developers accustomed to Ruby's Time.now.
And there are already a few other precedents, where we have essentially aliases.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

Please note the prior discussion about alias/deprecation in #5346 after the merge.

@bcardiff

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I would vouch for a deprecation cycle and a --check compiler option. There is little gain with this compile time error.

When possible the author of shards should be able to support two crystal versions at the same time to simplify migration paths.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

Following the discussion here and in crystal-lang/crystal-mysql#73, I've changed this PR to re-add Time.now for 0.28.0 as an alias to Time.local. It doesn't hurt to keep the alias for now and having a grace period makes it easier to migrate, improve developer happiness.

@j8r

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Only changing the method to an alias without any deprecation warning, and probably most users who don't read carefully the changelog won't notice the deprecation, even if it appears in the API docs.

Show resolved Hide resolved src/time.cr Outdated
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

@j8r Yes, that's what we want. For now at least. This is a breaking change, but given it's widespread use, it shouldn't immediately break every code. Let's give people time to adapt and keep support for both .local and .now simultaneously at least for one release (maybe even more).

I'd like to keep .now usable at least for 0.28.0. After that is released we can consider to completely remove it, add a compile time deprecation message (maybe already with compiler support for @[Deprecated]), postpone to the next release, or even decide that we want to keep this as an exceptional alias indefinitely.

@straight-shoota straight-shoota force-pushed the straight-shoota:feature/deprecate-time-now branch from 19c49c2 to 7808761 Mar 26, 2019

@asterite

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@straight-shoota What we can do is leave the alias but still output a warning at compile-time. The warning is annoying but your code will work. But because it's annoying people will change the new call to local or utc, and so when we introduce the breaking change people would have adapted already.

What do you think?

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

We had similar discussions about compiler warnings before, but I think it was largely agreed that the compiler should either work or fail, and don't print warnings.

Anyway, I'd say it's fine to merge this as is, adding a deprecation notice in the API docs. Let's see what the suggested compiler check feature brings and continue this discussion afterwards.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Added @[Deprecated] annotations from #7596.

Will only work after 0.28 is released.

@straight-shoota straight-shoota force-pushed the straight-shoota:feature/deprecate-time-now branch from df50707 to 7808761 Apr 4, 2019

@bcardiff

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@straight-shoota we can do

  {% if compare_versions(Crystal::VERSION, "0.28.0-0") >= 0 %}
    @[Deprecated("....")]
  {% end %}
  def foo
  end
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Let's just add that after 0.28.0. this PR can be merged without.

@bcardiff

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

But then the deprecation will exist from 0.29.
We are used for the std lib to have some checks that apply from the next release

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

I don't see what's the benefit of adding a version guard for a deprecation annotation instead of simply adding the deprecation annotation after the next release?

@bcardiff

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

That it could be removed in 0.29.0. That's the only benefit I see. Since we are adding it again for 0.28 for a migration path I thought we wanted to remove it as early as possible.

@asterite
Copy link
Member

left a comment

I'm thinking that we can add the annotation as a source file too in the top level, and even document it. Then we can use it right away and the next compiler will pick it up. What do you think? /cc @bcardiff

We should do the same too for other attributes (link, packed, etc)

straight-shoota added some commits Mar 25, 2019

src/time.cr Outdated
# Creates a new `Time` instance representing the current time from the
# system clock observed in *location* (defaults to local time zone).
#
# DEPRECATED: `Time.now` is deprecated, use `Time.local` or `Time.utc` instead.

This comment has been minimized.

Copy link
@bcardiff

bcardiff Apr 11, 2019

Member

We are ready for a rebase and remove the deprecated comment here and in the other method.

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Apr 11, 2019

Author Member

Forgot to push 🕺

@straight-shoota straight-shoota force-pushed the straight-shoota:feature/deprecate-time-now branch from 23fcb53 to ff255e8 Apr 11, 2019

Show resolved Hide resolved src/time.cr Outdated

@bcardiff bcardiff merged commit 51238b4 into crystal-lang:master Apr 11, 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:feature/deprecate-time-now branch Apr 11, 2019

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.