-
Notifications
You must be signed in to change notification settings - Fork 730
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
Suggestions for v2.0 #717
Comments
Thoughts:
|
Thoughts: 2 I though this came from the 2.0 branch. I cannot remember why I did it in my branch, although it seems reasonable. I think all I did was to update the tests (24 occurrences) to use 5 I agree the name should be more explicit. The impact of an 6 Since I believe 90% of the case apply a single unit, this version seems more readable as an English sentence. At least that's what I wrote first before looking at the documentation. 9 makes sense 11 |
2 - ok with me then 6 - thinking back to when I first decided this. One big reason I didn't do this is that it encourages chaining the operations like 11 - what about just being even more explicit: |
for 11. I think i still prefer |
Notes: 7. affects 8. I actually chose to remove the Locale options altogether. It makes For completeness in the v2.0 changelog,
Only 5. and 11. left, which may require some thought. I like |
Regarding 5., Looking at it today, it took me a while to understand the real meaning of this option. The time zone in the string (if present) is taken into account to compute the proper associated timestamp. However, the created DateTime object is defined in the This is a complex behavior, that could be detailed in the documentation. The parameter could be renamed Just like 8. above, which mixed two functionalities in its options (define the output format, but also alter the locale), it seems easier to separate concerns, by removing this parameter altogether. That way, these 5 methods would have the same When the parsed string provides a time zone, it should simply be respected. Otherwise, the This is equivalent to what The documentation could then become: Two cons of this approach:
Pros:
|
Just found this issue, and thought I'd share my thoughts 🙂 I generally agree with previous decisions, except for the following:
This seems like a bad idea. It's not at all obvious which zone the instance will be created with, where
Agreed, the name That said, I'd be much happier if this was just true by default - when an instance is created from a date string that specifies an offset, it is super confusing that the instance doesn't have that same offset. Basically, I'd expect this to be true: const isoDate = "2020-01-01T00:00+05:00";
DateTime.fromISO(isoDate).toISO() === isoDate; // currently false, which is super confusing We literally set this option to true everywhere we parse dates, but it's a hassle to write, easy to forget, and a major source of bugs. It's a breaking change for sure, but in my opinion, it would be a really, really good one.
This one seems like a bad idea. As mentioned in previous comments, the order of operations matter, and this would likely encourage incorrect use. Also, introducing multiple ways to do the same thing, just creates unnecessary complexity and confusion. Please keep the API clean and simple - it's enough to have one way to do something :)
This one I have mixed feelings about. Getting the Unix time is one use case for this method, but it may not be the only one - and for other use cases, the loss of precision could be a surprise. This certainly needs to be clearly documented. That said, I think I'm in favor of changing
Regarding the proposed
|
Hmm, on a second thought...
If the only change we make, is to floor the number of seconds returned by const seconds = 1577818800.5;
DateTime.fromSeconds(seconds).toSeconds() === seconds; // currently true, which is good
// but, if we change it to floor the value, this would be false, and that would be super confusing Therefore, if we were to make this change, then Now, if you do need to parse fractional seconds, you can of course always do this: const date = DateTime.fromMillis(seconds * 1000) Ignore everything below this line - it is wrong, and only left here for contextBut, here's where it gets really fun - because Therefore, I'm now in favor of keeping the current behavior of As for how to get the floored value... I believe what is really being asked for here, is in fact a way to get what is commonly known as "Unix time". This does raise a couple of interesting question though:
So, to summarize my thoughts on this point, I'd suggest:
|
Sorry, looks like I managed to confused myself in the previous comment 😄 The So, to summarize my thoughts on this, I suggest the following:
Additionally:
|
To make sure this doesn't get missed, I commented some thoughts about this on the original thread: #565 (comment) |
I have just talked with @joepie91 in the IRC about it. I’m not as seasoned as most of you guys, but I think the best solution is to rename the With renaming it it’s better defined what it does and if somebody wants the seconds without the ms they can still do |
I've opened a PR to add another function for whole number unix timestamps, it uses the same Math.floor method as Moment. |
These are ideas for potential breaking changes that could be introduced in v2.0, all open for discussion.
Settings.setDefaultZone
instead ofSettings.defaultZone
setter since input type differs from gettercreate()
instead to benefit from the cacheZone.universal
toisUniversal
DateTime.now()
method equivalent tolocal
with no parameteroptions.setZone
tokeepZone
inDateTime
constructors' optionsDateTime.plus/minus(12, 'hours')
methodplus/minus(ms)
, i.e. a number parameter which has no obvious meaning when reading the codeDateTime
,resolvedLocaleOpts
,toLocaleString
andtoLocaleParts
all accept anoptions
parameter which is a union ofLocaleOptions
(i.e.{ locale, outputCalendar, numberingSystem }
already used inreconfigure
andtoFormat
) andIntl.DateTimeFormatOptions
. These could be split into two options parameters instead, each with a different roleDateTime::resolvedLocaleOpts
returns an{ locale, outputCalendar, numberingSystem }
object, whereasIntl.ResolvedDateTimeFormatOptions
returns{ locale, calendar, numberingSystem (and more) }
.outputCalendar
could be renamed tocalendar
instead to be more consistent.DateTime::resolvedLocaleOpts
toresolvedLocaleOptions
DateTime.toSeconds
returns an integer value (see DateTime.toSeconds returns seconds.milliseconds #565), so that it can be used in Unix timestamps creation, and can otherwise be replaced bytoMillis() / 1000
The text was updated successfully, but these errors were encountered: