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

re #347 Add a system-level keyword to specify that COSMOS should glob… #457

Merged
merged 2 commits into from
May 23, 2017

Conversation

donaldatball
Copy link
Contributor

…ally use UTC times.

Note to reviewer: Under the hood, this implementation uses a monkey patch to the Time class so that Time objects created with new, now, and at are set up to be local or UTC depending on the system setting. I know touching core classes that way is a scary proposition, but this was the only way that I could come up with to consistently apply a Time Zone setting all across COSMOS. Another option would maybe be to derive a special CosmosTime class, but that seems like it'd be a lot of search'n'replace work, and hard to enforce usage in the future.

@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage decreased (-0.05%) to 85.751% when pulling 46d605e on system_time_zone into 9abc903 on master.

@ghost
Copy link

ghost commented May 19, 2017

Unfortunately can't monkey patch core Time methods without almost certainly breaking other ruby libraries. Will have to change to the massive search/replace option.

@donaldatball
Copy link
Contributor Author

@ryanatball

Fair enough, I figured this was pretty risky (although I did learn a lot).

A couple possible ideas:

  1. Create CosmosTime (better name?) class derived from Time; constructor, now, and at functions overridden (instead of overwritten). Replace all Time objects with CosmosTime objects.
    or
  2. Add .sys (better name?) function to core Time.class, operates on self. Replace Time.now with Time.now.sys, throughout the system (also Time.at with Time.at.sys, Time.new with Time.new.sys).

It feels like the first option is more likely to be correct in all cases. Do you have a preference? Or is there a better way that I'm not thinking of?

@ghost
Copy link

ghost commented May 19, 2017

Probably the CosmosTime option. I think you may need to make some more changes to the new() method as well. The changes you've made change how time will be formatted into a string, but not how time is entered. For example .new seems to always accept entry in local time units (probably needs to change to accommodate entry from UTC?). Also .at() always takes entry in UTC (probably should NOT change).

@coveralls
Copy link

coveralls commented May 23, 2017

Coverage Status

Coverage decreased (-0.03%) to 85.771% when pulling 996823f on system_time_zone into 9abc903 on master.

@donaldatball
Copy link
Contributor Author

@ryanatball

This should be ready for review now. I ended up going with the .sys() function in the Time class for a few reasons:

  1. It's the same amount of search'n'replace work, but less code elsewhere (i.e. just a couple functions in Time.rb as opposed to a whole new CosmosTime.rb file).
  2. I started feeling that it'd be better to have all of our hacks to the Time class in one place, so someone that's looking through our core_ext folder will see everything we've done.
  3. Even though it worked ok with respect to the UTC/local settings remaining consistent, it was weird having objects start out as CosmosTimes and then change back to regular Times-- now they're all just Time objects all the time.

@ghost
Copy link

ghost commented May 23, 2017

👍 Looks great Donald.

@donaldatball donaldatball merged commit b2dffdd into master May 23, 2017
@donaldatball donaldatball deleted the system_time_zone branch May 23, 2017 18:37
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.

2 participants