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

refrenceDate support for fromObject #1632

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

angelaki
Copy link

Analogous to date-fns's parse method, luxon should support a referenceDate, too.

Copy link

linux-foundation-easycla bot commented May 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

src/datetime.js Outdated Show resolved Hide resolved
@dasa
Copy link
Contributor

dasa commented May 17, 2024

refrenceDate -> referenceDate

Co-authored-by: Dasa Paddock <[email protected]>
@angelaki
Copy link
Author

So what are we waiting for? :)

@dasa
Copy link
Contributor

dasa commented May 29, 2024

I'm not sure about the logic, but there is a spelling typo I mentioned at: #1632 (comment)

src/datetime.js Outdated
Comment on lines 771 to 773
const tsNow = isUndefined(opts.refrenceDate)
? friendlyDateTime(opts.refrenceDate).toUnixInteger()
: opts.refrenceDate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a member of the Luxon team, but here's my thoughts:

  • maybe tsNow would be better named tsRef
  • I don't think opts.refrenceDate should be used if it's undefined

@angelaki
Copy link
Author

angelaki commented Jun 4, 2024

Fixed your concerns. Thank you pretty much. Time for a merge now? @Luxon-Team?

* @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'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it as it is for now then!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example here still contradicts the actual behavior and implementation.

expect(res.year).toBe(new Date().getFullYear());
expect(res.day).toBe(10);
});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to also add tests for:

  • Calling fromObject with actual data and check that the data is "merged in".
  • Checking that referenceDate is only used for units higher than the highest unit in the source object (i.e. fromObject({ day: 10 }) reads only year and month from the reference date.
  • Some tests for methods that indirectly call fromObject (like fromISO) with referenceDate.

src/datetime.js Show resolved Hide resolved
@diesieben07
Copy link
Collaborator

I'll do another review pass this evening!

@angelaki
Copy link
Author

angelaki commented Jun 12, 2024

@diesieben07 did this evening already happen? ;) Sorry for bumping, no hurries! Just kidding :) Just wanted to create a PR for a twoDigitCutoffYear parameter (for Settings being just used as fallback) but used my master branch for this fix. So it's kinda unhandy to create a new PR. My fault ...

Btw: needs @types/luxon be adopted, too? Or do these libraries get generated?

Copy link
Collaborator

@diesieben07 diesieben07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @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'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example here still contradicts the actual behavior and implementation.

@diesieben07
Copy link
Collaborator

Just wanted to create a PR for a twoDigitCutoffYear parameter (for Settings being just used as fallback) but used my master branch for this fix. So it's kinda unhandy to create a new PR. My fault ...

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.

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.

3 participants