Open aronsuarez opened 8 months ago
In my tests this works correctly. Are you setting useLocaleWeeks
when using startOf
? startOf('week')
on its own always uses the ISO week year.
In my opinion having to opt-in to use the default week settings (by passing useLocaleWeeks: true
into startOf
/endOf
) causes friction, is not intuitive and is not consistent with how the other Settings work.
Would it be possible to have the useLocaleWeeks
option default to true
? Or even better - replace the useLocaleWeeks
with a firstDayOfWeek
option in startOf
/endOf
?
Would it be possible to have the useLocaleWeeks option default to true?
No, because that would be a breaking change.
Or even better - replace the useLocaleWeeks with a firstDayOfWeek option in startOf/endOf?
Sure, I'd be fine with accepting a PR that adds this as an additional option. However this option not just applies to startOf
and endOf
. There are other functions which call startOf
and endOf
(such as hasSame
or Interval#count
) which will need their docs updated, too.
No, because that would be a breaking change.
Well, I assumed it would be the case.
And how about adding useLocaleWeeks
to WeekSettings?
With that option we could write this:
Settings.defaultWeekSettings = {
useLocaleWeeks: true,
firstDay: 7,
minimalDays: 4,
weekend: [6, 7],
};
DateTime.local(2024, 9, 10).startOf('week');
which would be equivalent to this:
Settings.defaultWeekSettings = {
firstDay: 7,
minimalDays: 4,
weekend: [6, 7],
};
DateTime.local(2024, 9, 10).startOf('week', { useLocaleWeeks: true });
I personally don't like this approach and would prefer to have as little "behavior altering Settings" as possible.
Settings.throwOnInvalid
has been a thorn in my eye ever since I started using Luxon, but it cannot be removed without a thorough change of how invalidity is handled, which will require a new major version. I would prefer it, if something like it weren't added.
The same goes for Settings.twoDigitCutoffYear
.
I fully support the point about avoiding settings which change the API contract, for example Settings.throwOnInvalid
and Settings.twoDigitCutoffYear
.
At the same time I find the settings which define the locale-dependent preferences extremely useful, for example Settings.defaultZone
, Settings.defaultLocale
, Settings.defaultNumberingSystem
and Settings.defaultOutputCalendar
.
In my opinion Settings.defaultWeekSettings
is another example of a very useful locale-dependent preference.
The problem I see with Settings.defaultWeekSettings
is that it is not used by default like the other Settings.default*
preferences. The fact that I need to opt-in to use the default week settings every time I call startOf
/endOf
(by passing in { useLocaleWeeks: true }
) removes a major benefit of Settings.defaultWeekSettings
.
The minimum I'm trying to achieve is to be able to call dateTime.startOf('week')
/dateTime.endOf('week')
without any extra params and with the first day of the week defined by Settings.defaultWeekSettings.firstDay
. Ultimately I'd like Settings.defaultWeekSettings
to be handled consistently with the other Settings.default*
settings, in the sense that Settings.defaultWeekSettings
are used by default with the ability to override them on a case by case basis.
Here's how I think we could approach it.
Add useLocaleWeeks
to WeekSettings, so that we could avoid having to pass { useLocaleWeeks: true }
into startOf
/endOf
every time. While not perfect, this step solves the main issue (Settings.defaultWeekSettings
not used by default) in a very simple and backward compatible way.
firstDayOfWeek: number
option to DateTime#startOf
, DateTime#endOf
and Interval#count
. This will allow users to override the week settings on a case by case basis, which would also address https://github.com/moment/luxon/issues/1573.useLocaleWeeks
in Settings.defaultWeekSettings
, DateTime#startOf
, DateTime#endOf
and Interval#count
.useLocaleWeeks
from Settings.defaultWeekSettings
, DateTime#startOf
, DateTime#endOf
and Interval#count
.useLocaleWeeks: boolean
will be completely gone.DateTime#startOf
, DateTime#endOf
and Interval#count
will support firstDayOfWeek: number
.firstDayOfWeek: number
option in DateTime#startOf
, DateTime#endOf
and Interval#count
will default to Settings.defaultWeekSettings.firstDay
, with the fallback to 1
, if the setting is not provided.What are your thoughts about the above? How would you approach avoiding the need for { useLocaleWeeks: true }
in startOf
/endOf
calls?
useLocaleWeeks
is not a locale-dependent setting. It changes how startOf/endOf works. If enabled, it uses weeks in terms of the locale, if disabled it uses the ISO week year system.
It will never be completely removed, because that would be removing a feature (using ISO weeks).
How would you approach avoiding the need for { useLocaleWeeks: true } in startOf/endOf calls?
I wouldn't. You don't always want locale-based weeks.
Could Luxon allow developers to set a preference globally to determine, if they want locale-based or ISO-based weeks by default, without having to set it every time when calling DateTime#startOf
, DateTime#endOf
and Interval#count
?
That is exactly what I would like to avoid as explained in my previous comment.
To me this is the same as globally configuring that startOf
suddenly by default always uses startOf('day')
. Global configuration like this makes it hard to reason about what your application is doing and makes it hard to write code that depends on Luxon and works in the presence of other code that also uses Luxon.
If I am a library author and my library is supposed to work in conjunction with Luxon, I would no longer be able to rely on Luxon's API, because users can just configure it to operate differently.
I see now that adding Settings.defaultWeekSettings.useLocaleWeeks
would be a breaking change. I'm not going to push it further but please consider the following when planning the next major release of Luxon:
Settings.defaultWeekSettings
should be consistent with the other Settings.default*
preferences, in particular:
Settings.defaultWeekSettings
should apply by default (without having to opt-in using useLocaleWeeks: true
).Settings.defaultWeekSettings
should be possible to override on a case-by-case basis using function params, for example useLocaleWeeks
(or firstDayOfWeek
).Thank you for the great insights and quick replies.
Settings.defaultWeekSettings should apply by default (without having to opt-in using useLocaleWeeks: true).
That is not what useLocaleWeeks
does! useLocaleWeeks
does not say "Use Settings.defaultWeekSettings
". useLocaleWeeks
says "Use weeks based on the locale, not standard ISO weeks". By default, this looks at the device's locale. defaultWeekSettings
overrides the device's locale, similar to how defaultZone
overrides what the default time zone of the device is. Configuring defaultZone
has no effect on things that do not use the local time zone and in exactly the same way defaultWeekSettings
has no effect on things that do not use the local week configuration.
Settings.defaultWeekSettings should be possible to override on a case-by-case basis using function params, for example useLocaleWeeks (or firstDayOfWeek).
A pull request for this would have my support for sure.
Describe the bug Settings.defaultWeekSettings has no effect at all, the weeks are starting still at Sunday
To Reproduce Settings.defaultLocale = 'de-CH' Settings.defaultWeekSettings = { firstDay: 1, minimalDays: 1, weekend: [6, 7] }
Actual vs Expected behavior I want to set default start day of the week to monday, currently is nothing changed when i set the Settings.defaultWeekSettings
Desktop (please complete the following information):
Additional context Add any other context about the problem here.