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

Should Rails/TimeZone be marked as unsafe cop? #6950

Closed
vfonic opened this issue Apr 20, 2019 · 3 comments
Closed

Should Rails/TimeZone be marked as unsafe cop? #6950

vfonic opened this issue Apr 20, 2019 · 3 comments

Comments

@vfonic
Copy link
Contributor

vfonic commented Apr 20, 2019

I just spent some time debugging my app trying to figure out why, after the last set of changes, my timezones are all messed up.

I ran git bisect which led me to this:

9be16555a4a3d4e64de2606cfdaac07fcf4d6805 is the first bad commit
commit 9be16555a4a3d4e64de2606cfdaac07fcf4d6805
Author: Viktor Fonic <viktor.fonic@gmail.com>
Date:   Wed Apr 3 23:56:17 2019 +0900

    Run `rubocop -DRES --safe-auto-correct`

So, as the commit message states, I only ran rubocop -DRES --safe-auto-correct. This introduced a bug in my app that I still didn't track down (due to this command having changed 89 files (with 389 additions and 232 deletions). Most of these changes are adding # frozen_string_literal: true, but...I digress.

This is just really frustrating. I ran the above command and never looked back. However, safe autocorrect broke my app. What's the point of having both safe autocorrect and "just autocorrect" if both are unsafe?

Please, please, let's mark this cop as unsafe until we figure out how to make it safe (if possible). I'd also suggest this for all cops. --safe-auto-correct in my opinion should NEVER break the code. Ok, ok, ruby is dynamic language. But, in most normal cases, it shouldn't break.

I'll edit this issue and report back once I figure out which exact change introduced the issue with timezones.

Sorry for the rant. ❤️

EDIT:
Here's what happened:

In my app, I show a timeline graph to the user. I request the graph via AJAX after the page loads. The backend controller gets params[:now] and uses that (user's browser-detected timezone), to calculate beginning and end of day:

# code simplified for demo purposes
def current_day_span
  # params[:now] currently returns "2019-04-20T14:29:19+07:00"
  now = Time.parse(params[:now])
  [now.beginning_of_day, now.end_of_day]
end

After running rubocop --safe-auto-correct, this has been changed to:

def current_day_span
- now = Time.parse(params[:now])
+ now = Time.parse(params[:now]).in_time_zone
  [now.beginning_of_day, now.end_of_day]
end

This now returns:
[Sat, 20 Apr 2019 00:00:00 UTC +00:00, Sat, 20 Apr 2019 23:59:59 UTC +00:00]
as opposed to:
[2019-04-20 00:00:00 +0700, 2019-04-20 23:59:59 +0700]

I understand, I could make the code work by extracting the timezone from the :now param, but what's the point? The code I initially wrote, worked just fine until the safe cop broke it. 🤷‍♂️

Halp!

What are you thoughts?

@vfonic vfonic changed the title Rails/TimeZone is unsafe cop Rails/TimeZone in strict mode is unsafe cop Apr 20, 2019
@vfonic vfonic changed the title Rails/TimeZone in strict mode is unsafe cop Should Rails/TimeZone be marked as unsafe cop? Apr 20, 2019
@matkoniecz
Copy link

matkoniecz commented Apr 23, 2019

Should Rails/TimeZone be marked as unsafe cop?

Given that this cop may (will?) change what data returned by a normal code it certainly should be marked as am unsafe.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 24, 2019

Let's mark it as unsafe.

@vfonic
Copy link
Contributor Author

vfonic commented Apr 25, 2019

I created a PR that marks it as unsafe: #6966

vfonic added a commit to vfonic/rubocop that referenced this issue Apr 25, 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

No branches or pull requests

3 participants