-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
Add Date module #2799
Add Date module #2799
Conversation
🦋 Changeset detectedLatest commit: 42d8257 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Hi, thanks for the PR :) Tackling a
You can add
I think what you have there is fine.
It will be reviewed on a case by case basis. If we feel there are tests missing during review, we will let you know.
We will give your PR a proper review soon. A |
7a93c23
to
e975f0e
Compare
7a29d3c
to
838807b
Compare
b6e0233
to
11d8c90
Compare
11d8c90
to
ada12f1
Compare
Hi @tim-smart , is there anything I need to do to move this along, or still just waiting for code review? I see one of the GitHub checks fails but I'm not sure what causes it. |
Sorry for the neglect, but I think we just need to set aside some time to think about how we want to approach dates in effect. Maybe we could introduce a js Date also has so many footguns it might be better to side step it and create a more type safe data structure. The upcoming Temporal proposal could be used as inspiration: https://tc39.es/proposal-temporal/docs/index.html |
6d6d9bf
to
1282502
Compare
I had a play around with the domain here: #3296 Not sure about the direction yet, but I think putting up some guard rails is a good approach. |
Thanks Tim. Agreed on shipping only UTC functions - there's a lot to keep in mind with the local stuff 😅 Your domain looks like a great start. Do you want me to move over the UTC getters, setters, and math functions from this PR into that? (Not sure how this fits with the WithZone part of it though - is that basically just an offset on top of UTC?) |
I think for time zones we can lean on the Yeah feel free to pitch in. For the math functions it could be quite nice to have something like: DateTime.add(self, 6, "months")
DateTime.add(self, Duration) Then we can just "do the right thing" to prevent issues when crossing over daylight savings. |
f9882d2
to
62ce405
Compare
Made some solid progress. Will close this in favour of the other PR. |
Type
Description
Add Date module.
Related
N/A
Questions from me
Hi, my first time contributing to this! Hopefully this module is valuable and well-written.
I had some questions about code style and documentation.
First -
@since
tags - who adds these? I don't think I can do it since I don't know which version this might be added to.Second - Predicate-based dual api functions. I've never seen these in Effect, but there are a few functions that offer optional arguments with default values, such as the week functions and their
dowOffset
. There's no difference between these usages:...and TypeScript has no problem figuring out which overload to use, but it's a bit confusing to read.
Third - Is there some requirement for test coverage? I've added tests for everything that might be considered complex, but a lot of functions are also extremely trivial.
Fourth - I've added a lot in here and surely cover all the basics, but there's surely also more inspiration I could take from projects like
date-fns
. How comprehensive is something like this expected to be before it's accepted?Thanks!