refrenceDate support for fromObject#1632
Conversation
|
|
Co-authored-by: Dasa Paddock <dpaddock@esri.com>
|
So what are we waiting for? :) |
|
I'm not sure about the logic, but there is a spelling typo I mentioned at: #1632 (comment) |
| const tsNow = isUndefined(opts.refrenceDate) | ||
| ? friendlyDateTime(opts.refrenceDate).toUnixInteger() | ||
| : opts.refrenceDate, |
There was a problem hiding this comment.
I'm not a member of the Luxon team, but here's my thoughts:
- maybe
tsNowwould be better namedtsRef - I don't think
opts.refrenceDateshould be used if it'sundefined
Fixed isUndefined usage
|
Fixed your concerns. Thank you pretty much. Time for a merge now? @Luxon-Team? |
| * @param {DateTime|Date|Object} opts.referenceDate - the reference date to take for missing parts | ||
| * @example DateTime.fromObject({ year: 1982, month: 5, day: 25}).toISODate() //=> '1982-05-25' | ||
| * @example DateTime.fromObject({ year: 1982 }).toISODate() //=> '1982-01-01' | ||
| * @example DateTime.fromObject({ year: 1982 }, { referenceDate: { day: 10 } }).toISODate() //=> '1982-01-10' |
There was a problem hiding this comment.
This example is actually wrong. The referenceDate is only used for units above the highest unit in the input. If { month: 6 } is given, only the year is taken from the reference date.
There was a problem hiding this comment.
Oh sorry, I just expected the old tsNow to add all the missing parts (not just the units above). What's your opinion here? Change the logic on a passed refDate, so all missing parts are replaced? And if, always (could be a breaking change?) or just with passed refDate?
date-fns even uses proximity to guess the right input: https://date-fns.org/v3.6.0/docs/parse
There was a problem hiding this comment.
If that is added, it definitely needs to be opt in. DateTime.fromObject({ year: 2013 }) shouldn't suddenly have the current time.
What's your use-case for this?
There was a problem hiding this comment.
Tbh, I have no use-case for it. Just thinking, in an Englisch format, if only this first digits (month) is set, maybe someone wants to have a different day than the first (and not only a different year than todays?). But again, no hard feeling here! Can just stay as it is!
There was a problem hiding this comment.
Let's keep it as it is for now then!
There was a problem hiding this comment.
The example here still contradicts the actual behavior and implementation.
| expect(res.year).toBe(new Date().getFullYear()); | ||
| expect(res.day).toBe(10); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I think it would be good to also add tests for:
- Calling
fromObjectwith actual data and check that the data is "merged in". - Checking that
referenceDateis only used for units higher than the highest unit in the source object (i.e.fromObject({ day: 10 })reads onlyyearandmonthfrom the reference date. - Some tests for methods that indirectly call
fromObject(likefromISO) withreferenceDate.
|
I'll do another review pass this evening! |
|
@diesieben07 did this evening already happen? ;) Sorry for bumping, no hurries! Just kidding :) Just wanted to create a PR for a Btw: needs |
There was a problem hiding this comment.
I apologize for the delay on getting back to you here.
Unfortunately it seems you haven't really addressed the problems I highlighted in my previous review regarding tests and the wrong example.
Also, please resolve the merge conflicts that exist now.
| * @param {DateTime|Date|Object} opts.referenceDate - the reference date to take for missing parts | ||
| * @example DateTime.fromObject({ year: 1982, month: 5, day: 25}).toISODate() //=> '1982-05-25' | ||
| * @example DateTime.fromObject({ year: 1982 }).toISODate() //=> '1982-01-01' | ||
| * @example DateTime.fromObject({ year: 1982 }, { referenceDate: { day: 10 } }).toISODate() //=> '1982-01-10' |
There was a problem hiding this comment.
The example here still contradicts the actual behavior and implementation.
You can still create a new branch in your repo that tracks Luxon's master branch, they don't have to be named the same. |
3 participants
Analogous to date-fns's
parsemethod, luxon should support areferenceDate, too.