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

Rails/TimeZone: DateTime.new is wrongly corrected to DateTime.zone.new #67

Closed
doloopwhile opened this issue Jun 5, 2019 · 4 comments · Fixed by #85
Closed

Rails/TimeZone: DateTime.new is wrongly corrected to DateTime.zone.new #67

doloopwhile opened this issue Jun 5, 2019 · 4 comments · Fixed by #85

Comments

@doloopwhile
Copy link

When EnforcedStyle is flexible(default), Rails/TimeZone cop will correct DateTime.new to DateTime.zone.new. It breaks codes because DateTime.zone is not defined by Rails.

See: https://api.rubyonrails.org/classes/DateTime.html

Expected behavior

  • DateTime.new is left as it is.
  • DateTime.new is corrected to Time.zone.new as EnforcedTyope: strict

Actual behavior

  • DateTime.new is corrected to DateTime.zone.new

Other DateTime's methods (now, local, parse, at, current) are also corrected to DateTime.zone.*.

Steps to reproduce the problem

$ gem install rubocop -v 0.71.0
$ gem install rubocop-rails -v 2.0
$ echo 'require: rubocop-rails' > .rubocop.yml

$ echo 'DateTime.new' > a.rb

$ rubocop --only Rails/TimeZone --auto-correct a.rb
Inspecting 1 file
C

Offenses:

a.rb:1:10: C: [Corrected] Rails/TimeZone: Do not use DateTime.new without zone. Use one of Time.zone.now, DateTime.current, DateTime.new.in_time_zone, DateTime.new.utc, DateTime.new.getlocal, DateTime.new.xmlschema, DateTime.new.iso8601, DateTime.new.jisx0301, DateTime.new.rfc3339, DateTime.new.httpdate, DateTime.new.to_i, DateTime.new.to_f instead. (https://github.com/rubocop-hq/rails-style-guide#time, http://danilenko.org/2012/7/6/rails_timezones)
DateTime.new
         ^^^

1 file inspected, 1 offense detected, 1 offense corrected

$ cat a.rb
DateTime.zone.new

RuboCop version

$ rubocop -V
0.71.0 (using Parser 2.6.3.0, running on ruby 2.6.2 x86_64-darwin17)
@doloopwhile doloopwhile changed the title DateTime.new is wrongly corrected to DateTime.zone.new Rails/TimeZone: DateTime.new is wrongly corrected to DateTime.zone.new Jun 5, 2019
@tejasbubane
Copy link
Contributor

tejasbubane commented Jun 6, 2019

rubocop/rubocop#6930 says Prefer Time class over DateTime in strict mode. This works for strict mode where it changes to Time.zone but in non-strict mode this is a bug.

@tejasbubane
Copy link
Contributor

I think there is some confusion related to this cop's behaviour. cc: @vfonic

@vfonic
Copy link
Contributor

vfonic commented Jun 6, 2019

@tejasbubane thanks for mentioning me.

This is indeed a bug.

I think the easiest solution is to make the cop change DateTime to Time, and it moves us closer to having only one mode:
rubocop/rubocop#6930 (comment)

@doloopwhile thanks for the repro steps! The issue you explained seemed to work for me, until I followed the exact steps and then I saw the bug. Without the detailed repro steps, I would've replied (mistakenly) that the cop works for me and you should check your configuration. Repro steps are important. :)

I'm going to submit the PR for this now.

/cc @bbatsov @jonas054

vfonic added a commit to vfonic/rubocop-rails that referenced this issue Jun 6, 2019
This removes potential (and actual) bugs due to some methods not being present on `DateTime` class that are present on `Time` class.
If we always autocorrect to `Time`, we ensure the most appropriate correction.

rubocop#67
rubocop/rubocop#6930 (comment)
koic added a commit to koic/rubocop-rails that referenced this issue Jul 5, 2019
Fixes rubocop#67.
Closes rubocop#71.

This PR fixes an incorrect auto-correct for `Rails/TimeZone`
when using `DateTime`.

```diff
- DateTime.new
+ DateTime.zone.new
```

```ruby
DateTime.zone.new # NoMethodError: undefined method `zone' for
DateTime:Class
```

This PR changes to ignore `DateTime` with the following as
background.

`DateTime` is not mentioned in The Rails Style Guide and
`Rails/TimeZone` cop's examples.

- https://rails.rubystyle.guide/#time-now
- https://docs.rubocop.org/projects/rails/en/stable/cops_rails/#railstimezone

Recently, rubocop#81 and rubocop#82 have been feedback.
I think breaking by auto-correction should be fixed with
the highest priority in this case.

Also, `DateTime` and `Time` are not replaced from `DateTime` to `Time`
by auto-correct because they are different objects.
@koic koic closed this as completed in #85 Jul 6, 2019
koic added a commit that referenced this issue Jul 6, 2019
…ls_time_zone

[Fix #67] Fix an incorrect auto-correct for `Rails/TimeZone`
@koic
Copy link
Member

koic commented Jul 6, 2019

RuboCop Rails 2.2.0 has been released with a fix for this issue.
https://rubygems.org/gems/rubocop-rails/versions/2.2.0

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants