Change/timezone locale #6
Conversation
Support legacy systems that depend on this lib
lib/index.js
Outdated
| options.locale = locale; | ||
| } else if (locale) { | ||
| options = locale; | ||
| } |
There was a problem hiding this comment.
I'm a bit confused why we need this if/else statement?
This appears to be an addition to the API so why not just make every caller pass in an object.
There was a problem hiding this comment.
My original thinking was to mimic the second param of SugarDate.create which behaves like this. But I do like the idea of not having a mixed type as a parameter.
lib/index.js
Outdated
| * @return {Object} an object containing the parsed date, or the passed field if it was not a String. | ||
| */ | ||
| Utils.parseDateTimeField = function(field) { | ||
| Utils.parseDateTimeField = function(field, locale) { |
There was a problem hiding this comment.
See comment in Flow XO core, it would be good to allow the SugarDate creation to be customised for the lifetime of this function invocation by passing in an optional dateCreator fn.
There was a problem hiding this comment.
Since I wrote this comment, this feature has been implemented! So please ignore. 😄
lib/index.js
Outdated
| // Makes unit testing possible, by allowing this | ||
| // function to be mocked. | ||
| return Date.future(str); | ||
| return SugarDate.create(str, _.assign({future: true}, options)); |
There was a problem hiding this comment.
This would be better written as:
return SugarDate.create(str, _.assign({}, options, {future: true}));The advantage here is that an options object containing future: false would not cause issues.
There was a problem hiding this comment.
Ah! I see what you are saying.
Fixed a potential bug with gettign the future date.
`moment` is now returned and is a timezone aware moment.js object.
No description provided.