Skip to content

feat: implemented getters / setters and various string fns on Date #1768

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

Merged
merged 6 commits into from
Apr 14, 2021

Conversation

ColinEberhardt
Copy link
Contributor

This PR adds

  • getters / setters for UTC dates (I don't plan to add any local-time related functionality)
  • fromString, as a substitute for the new Date("2345-11-04") style constructor - I resisted the temptation to use as-regex for this task 😆
  • toISOString

I've also modified Date.UTC so that it no longer calls out to the JS host.


if (dateTimeString.includes("T")) {
// includes a time component
const parts = dateTimeString.split("T");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an opportunity here to make fromString basically allocation-free, by walking the string and using strtol to extract the values (currently doesn't support a limit, though). @MaxGraey Thoughts?

Copy link
Member

@MaxGraey MaxGraey Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, instead split better use indexOf + two substrings here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there would still be other avoidable allocations then when substringing the parts of interest into parseInt. Hence I thought this may be a case for slightly extending strtol so it stops at a given limit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this one to you @MaxGraey ;-)

Copy link
Member

@MaxGraey MaxGraey Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnbarak
Copy link
Contributor

bnbarak commented Mar 29, 2021

A tiny recommendations - add peace of mind for checking the odd corner cases in dates in different timezones. Perhaps add moment.js in devDependencies and add a test to compare the results of assemblyscript Date() type with moment.js Assuming moment.js is a source of truth to dates.

@ColinEberhardt
Copy link
Contributor Author

A tiny recommendations - add peace of mind for checking the odd corner cases in dates in different timezones.

Thanks @bnbarak - I don't plan to add any timezone related functionality to Date, because JS dates are horrible! Instead I am implementing the Temporal spec here:

https://github.com/ColinEberhardt/assemblyscript-temporal

The only reason I am adding UTC-related Date functionality here is that because I need that for Temporal ;-)

Copy link
Member

@dcodeIO dcodeIO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the merge conflict fixed. We may also want to update the docs accordingly as a follow-up, and do a pass on String#split afterwards :)

@ColinEberhardt
Copy link
Contributor Author

LGTM with the merge conflict fixed

Thanks @dcodeIO - rebased onto master - so should be good to merge when the tests pass 🤞

I'll update the docs after this is merged.

@dcodeIO dcodeIO merged commit 30f5d13 into AssemblyScript:master Apr 14, 2021
@MaxGraey MaxGraey mentioned this pull request Apr 14, 2021
1 task
ashutoshvarma added a commit to ashutoshvarma/near-sdk-as that referenced this pull request Apr 15, 2021
since AssemblyScript/assemblyscript#1768 is
merged we can safely remove relative import hack
ashutoshvarma added a commit to ashutoshvarma/near-sdk-as that referenced this pull request Apr 15, 2021
since AssemblyScript/assemblyscript#1768 is
merged we can safely remove relative import hack
willemneal pushed a commit to near/near-sdk-as that referenced this pull request Apr 15, 2021
since AssemblyScript/assemblyscript#1768 is
merged we can safely remove relative import hack
ColinEberhardt added a commit to ColinEberhardt/website that referenced this pull request Apr 19, 2021
MaxGraey pushed a commit to AssemblyScript/website that referenced this pull request Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants