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

New TimeZone should avoid explicit conversions #1770

Closed
jfelchner opened this issue Apr 7, 2015 · 4 comments
Closed

New TimeZone should avoid explicit conversions #1770

jfelchner opened this issue Apr 7, 2015 · 4 comments

Comments

@jfelchner
Copy link
Contributor

If the intention of the TimeZone cop is to keep people from accidentally working with a Time which has a Time Zone that they are not aware of, then explicit conversions via the utc method should be ignored since it doesn't matter what time zone Time.now returns if utc is chained onto it.

I can definitely see a situation where the user doesn't care what the Rails time zone is set to and they want to work explicitly with UTC times.

Proposal:

utc
localtime(offset)

should be marked as "safe" to use after Time.now and any other method marked as "dangerous" by this cop.

Rationale:

Time.now.utc while explicitly being cast to UTC, is not the same as Time.zone.now since Time.zone can be something other than UTC.

@jfelchner jfelchner changed the title New TimeZone should avoid explicit conversions to UTC New TimeZone should avoid explicit conversions Apr 7, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 8, 2015

//cc @palkan

@palkan
Copy link
Contributor

palkan commented Apr 8, 2015

Just to make sure I understand you correctly:

# good
Time.now.utc
Time.parse(some_time).localtime("+09:00")

# still bad
Time.now.localtime

@bbatsov
Do you think we should add this as 'always' or 'acceptable' style? I think the second.

palkan added a commit to palkan/rubocop that referenced this issue Apr 12, 2015
@ojab
Copy link
Contributor

ojab commented Apr 12, 2015

Second that, rubocop doesn't like Time.now.utc.strftime('%Y-%m-%d %H:%M:%S') here.

@jfelchner
Copy link
Contributor Author

@palkan to keep it consistent with what you currently have, I'd just mark it as acceptable.

palkan added a commit to palkan/rubocop that referenced this issue Apr 15, 2015
palkan added a commit to palkan/rubocop that referenced this issue Apr 15, 2015
palkan added a commit to palkan/rubocop that referenced this issue Apr 15, 2015
palkan added a commit to palkan/rubocop that referenced this issue Apr 16, 2015
palkan added a commit to palkan/rubocop that referenced this issue Apr 18, 2015
palkan added a commit to palkan/rubocop that referenced this issue Apr 18, 2015
palkan added a commit to palkan/rubocop that referenced this issue Apr 19, 2015
palkan added a commit to palkan/rubocop that referenced this issue Apr 20, 2015
bbatsov added a commit that referenced this issue Apr 20, 2015
[Fix #1770] Add utc/localtime support to Rails/TimeZone
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

4 participants