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

[Fix #67] TimeZone: Always autocorrect DateTime => Time #71

Closed
wants to merge 1 commit into from

Conversation

vfonic
Copy link
Contributor

@vfonic vfonic commented 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.

#67
rubocop/rubocop#6930 (comment)

Sidenote: I believe a lot of problems here also comes from the tests that are a little bit hard to follow. The tests seem to have been written for code efficiency / lower line numbers. But they make it hard to understand which test refers to which of the two classes (Time or DateTime) and what exactly is being tested:

Why are "accepted methods" registering offenses:

      described_class::ACCEPTED_METHODS.each do |a_method|
        it "registers an offense #{klass}.now.#{a_method}" do
          inspect_source("#{klass}.now.#{a_method}")
          expect(cop.offenses.size).to eq(1)
          expect(cop.offenses.first.message).to include('`Time.zone.now`')
        end
      end

Etc.
If we decide to keep only one mode (as opposed to ['flexible', 'strict'], I believe the refactoring should start from the tests.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

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)
@doloopwhile
Copy link

doloopwhile commented Jun 10, 2019

@vfonic
Should we do both of DateTime => Time and Time.now => Time.current be done is one Cop ?

They have different "stabilitiy".

  • Time.now == Time.current in almost Rails projects (as long as developer do not override default configuration).
  • Time.now != DateTime.now in all projects.

My suggestion is:

  • To make Rails/TimeZone Cop to care only Time and to ignore DateTime
  • And to create new Cop which warns usage of DateTime and replace DateTime => Time (Rails/DateTime ?)

@vfonic
Copy link
Contributor Author

vfonic commented Jun 10, 2019

Should we do autocorrection DateTime => Time and autocorrection Time.now => Time.current be done is one Cop ?

My suggestion is:

Make Rails/TimeZone Cop to care only Time and ignore DateTime
and create new Cop which warns usage of DateTime and corrects DateTime => Time (Rails/DateTime ?)

🤔 This is a good question.
There are two "algorithms" for checking what needs to be changed. However, we are changing the same semantic ("make sure you use time with zone") in both cases.
I think it's better if we keep these together. (DateTime => Time and Time.now => Time.current)

  • Time.now == Time.current in almost Rails projects (as long as developer do not override default configuration).

It's based on the machine's timezone. I understand most of production servers would have timezone in UTC, but do we really want to rely on that? That's why Time.now is not recommended. Also, using Time.now produces different results in production and development (time zone on my machine is not UTC).

I'll try to find the time to rewrite this cop, starting from what we want to have, in tests. (TDD ALL THE WAY!!! 😆)

What do you think?

@koic
Copy link
Member

koic commented Jun 27, 2019

Make Rails/TimeZone Cop to care only Time and ignore DateTime

Yeah, Clould you please update it to ignore DateTime? DateTime is not mentioned in the Rails Style Guide and Rails/TimeZone cop's examples.

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

I'm planning to release a patch version after the fix. Thank you for your consideration and discussion.

@vfonic
Copy link
Contributor Author

vfonic commented Jun 27, 2019

I'm sorry I'm short on time lately. :(

I suggest we convert both DateTime and Time to Time methods that respect the timezone.
Feel free to take over.
Thanks!

koic added a commit to koic/rubocop-rails that referenced this pull request 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 in #85 Jul 6, 2019
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 this pull request may close these issues.

None yet

3 participants