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.

Sign up for GitHub

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

Jump to bottom

refrenceDate support for fromObject #1632

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

refrenceDate support for fromObject #1632

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
886c801
`refrenceDate` support for `fromObject`
angelaki May 17, 2024
c459f39
Update src/datetime.js
angelaki May 17, 2024
b655ae7
Fixes typo `refrenceDate` --> `referenceDate`
angelaki Jun 4, 2024
8e568e6
Rename tsNow --> tsRef
angelaki Jun 4, 2024
448be0e
opts.referenceDate description on all methods relying on `fromObject`
angelaki Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/datetime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,8 @@ function normalizeUnitWithLocalWeeks(unit) {
// single timestamp for all zones to make things a bit more predictable.
//
// This is safe for quickDT (used by local() and utc()) because we don't fill in
// higher-order units from tsNow (as we do in fromObject, this requires that
// offset is calculated from tsNow).
// higher-order units from tsRef (as we do in fromObject, this requires that
// offset is calculated from tsRef).
function guessOffsetForZone(zone) {
if (!zoneOffsetGuessCache[zone]) {
if (zoneOffsetTs === undefined) {
Expand Down Expand Up @@ -745,8 +745,10 @@ export default class DateTime {
* @param {string} [opts.locale='system\'s locale'] - a locale to set on the resulting DateTime instance
* @param {string} opts.outputCalendar - the output calendar to set on the resulting DateTime instance
* @param {string} opts.numberingSystem - the numbering system to set on the resulting DateTime instance
* @param {DateTime|Date|Object} opts.referenceDate - the reference date to take for missing parts
diesieben07 marked this conversation as resolved.
Show resolved Hide resolved
* @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.

* @example DateTime.fromObject({ hour: 10, minute: 26, second: 6 }) //~> today at 10:26:06
* @example DateTime.fromObject({ hour: 10, minute: 26, second: 6 }, { zone: 'utc' }),
* @example DateTime.fromObject({ hour: 10, minute: 26, second: 6 }, { zone: 'local' })
Expand All @@ -766,10 +768,12 @@ export default class DateTime {
const normalized = normalizeObject(obj, normalizeUnitWithLocalWeeks);
const { minDaysInFirstWeek, startOfWeek } = usesLocalWeekValues(normalized, loc);

const tsNow = Settings.now(),
const tsRef = isUndefined(opts.referenceDate)
? Settings.now()
: friendlyDateTime(opts.referenceDate).toUnixInteger(),
offsetProvis = !isUndefined(opts.specificOffset)
? opts.specificOffset
: zoneToUse.offset(tsNow),
: zoneToUse.offset(tsRef),
containsOrdinal = !isUndefined(normalized.ordinal),
containsGregorYear = !isUndefined(normalized.year),
containsGregorMD = !isUndefined(normalized.month) || !isUndefined(normalized.day),
Expand Down Expand Up @@ -797,7 +801,7 @@ export default class DateTime {
// configure ourselves to deal with gregorian dates or week stuff
let units,
defaultValues,
objNow = tsToObj(tsNow, offsetProvis);
objNow = tsToObj(tsRef, offsetProvis);
if (useWeekData) {
units = orderedWeekUnits;
defaultValues = defaultWeekUnitValues;
Expand Down
6 changes: 6 additions & 0 deletions test/datetime/create.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,12 @@ test("DateTime.fromObject takes a undefined to mean {}", () => {
expect(res.year).toBe(new Date().getFullYear());
});

test("DateTime.fromObject respects `referenceDate`", () => {
const res = DateTime.fromObject(undefined, { referenceDate: { day: 10 } });
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.

test("private language subtags don't break unicode subtags", () => {
const res = DateTime.fromObject(
{},
Expand Down