-
Notifications
You must be signed in to change notification settings - Fork 30
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 locale and formatting options to localeDateString, localeString and localTimeString functions #30
Add locale and formatting options to localeDateString, localeString and localTimeString functions #30
Conversation
…nd localTimeString functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff, thank you very much!
2 small things I wonder about:
- Could you add a comment somewhere on what things from the spec you left out? In case someone wants to pick that up.
- Could you add docstrings for the things you've added? I know the rest of the file doesn't have docstrings yet (we'll fix that), but it'd be nice to include it directly for anything we add.
@zth I tried to generate an interface file to add the doc strings there, but it seems that the command For my additions I could write it by hand but I don't want to "hide" all other functions. |
There's a command in the VSCode extension that can generate interface files, I use that. |
Okay, thanks :) I will install vscode for that 🙄 Todo for me: I have to figure out, how I can implement it in the vim plugin. |
Yeah that would be good to have in vim too for sure. |
So, I added the doc strings. Wasn't sure about formatting. Existing documentation is indented, the documentation in open PRs isn't. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great docstrings! Just a few small adjustments and this is good to go. Thank you!
src/Core__Date.resi
Outdated
|
||
## Examples | ||
```rescript | ||
Date.make()->Date.toLocaleDateString->Console.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a comment with what the output will be when you log to the console here? Or rather, what the output is when you do it now as you're adding the docstrings.
This goes for all the examples.
src/Core__Date.resi
Outdated
A type representing date time format options. | ||
|
||
Note: There are some properties missing: | ||
- fractionalSecondDigits | ||
- dayPeriod | ||
- calendar | ||
- numberingSystem | ||
- localeMatcher | ||
- timeZone | ||
- hour12 | ||
- hourCycle | ||
- formatMatcher | ||
|
||
See full spec at https://tc39.es/ecma402/#datetimeformat-objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the indentation here.
9411553
to
20f5535
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, great work, thank you! Merging.
Would you potentially be up for adding similar docstrings for the rest of Date in a separate PR?
I was just thinking about it too. I will ;) |
I didn't implement the full spec, but added hopefully the most important options.
I took this article as source https://medium.com/swlh/use-tolocaledatestring-to-format-javascript-dates-2959108ea020