-
Notifications
You must be signed in to change notification settings - Fork 46
Allow parsing timestring at different timezones. #58
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
base: master
Are you sure you want to change the base?
Conversation
|
Better like this? |
| (parse-timestring "2016-12-01" :offset nil) ;; Use default timezone | ||
| (parse-timestring "2016-12-01" :offset (* 60 60 -5)) ;; Ignore timezone | ||
| (parse-timestring "2016-12-01" :offset (* 60 60 -5) :timezone utc-leaps) ;; Ignore timezone | ||
| (parse-timestring "2016-12-01" :offset nil :timezone eastern-tz) ;; Use timezone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be wrapped in a (let ((default-timezone +utc-zone+)) ...) because you're not actually testing here that the timezone used is different from the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this were in UTC zone, parse-timestring might change such that :offset nil would be interpreted as :offset 0 regardless of the *default-timezone*, whereas this code tests that :offset nil uses *default-timezone*.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I've let this PR seriously languish. Sorry.
I mean that by setting default-timezone to eastern-tz and carefully arranging the arguments so that they return the same timestamps, this test basically doesn't show that the arguments do anything at all. A version of parse-timestring that ignored all its arguments and only used default-timezone would pass this test.
| (parse-timestring "2016-12-01" :offset (* 60 60 -5)) ;; Ignore timezone | ||
| (parse-timestring "2016-12-01" :offset (* 60 60 -5) :timezone utc-leaps) ;; Ignore timezone | ||
| (parse-timestring "2016-12-01" :offset nil :timezone eastern-tz) ;; Use timezone | ||
| (adjust-timestamp (parse-timestring "2016-12-01" :offset nil :timezone utc-leaps) (offset :hour +5)) ;; Use different timezone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems more of a test of adjust-timestamp.
| (new (timestamp+ old 24 :hour eastern-tz))) | ||
| (is (= (* 24 60 60) (timestamp-difference new old))))) | ||
|
|
||
| (deftest test/timezone/parse-timestring-different-timezones () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're testing timezones but only using date timestamps? That seems odd.
dlowe-net
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even know how to github anymore.
Allow parse-timestring to parse timestring in different timezones. This would be useful in cases where timezone is sent separately like it could (and often does) in the icalendar format.
As discussed in #56