-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
Conversation
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)
@vfonic They have different "stabilitiy".
My suggestion is:
|
🤔 This is a good question.
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 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? |
Yeah, Clould you please update it to ignore
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. |
I'm sorry I'm short on time lately. :( I suggest we convert both |
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.
This removes potential (and actual) bugs due to some methods not being present on
DateTime
class that are present onTime
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
orDateTime
) and what exactly is being tested:Why are "accepted methods" registering offenses:
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:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).and description in grammatically correct, complete sentences.
bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.