Skip to content

fix: Improve date parsing (fractional seconds, UTC offsets) #2803

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 10 commits into from
Nov 20, 2023

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Nov 18, 2023

The logic used by Date.fromString and Date.parse has a few issues:

  • Incorrect handling of fractional seconds having more than 3 decimal places. For example, "2000-01-01T00:00:00.123456789Z" was being treated as having 123456789 milliseconds, making the result wildly too large. Such values should be truncated to 3 decimal places before being interpreted as milliseconds.
  • Incorrect handling of fractional seconds having less than 3 decimal places. For example "2000-01-01T00:00:00.1" was treated as having 1 millisecond, though it should be one tenth of a second (or 100 milliseconds).
  • Ignoring any UTC offset (such as +05:30 or -08:00), which is required by ISO 8601, RFC 3339, and the ECMAScript standard date format.

This PR addresses all three issues, and updates the existing Date#fromString test.

I also made two small unrelated changes:

  • Replaced usages of I32.parseInt with i32.parse in std/assembly/date.ts to resolve deprecation warnings
  • Regenerated the fixture for std/math test to resolve "fixture mismatch" error that was occurring on a clean clone of the repo before I made any changes. (Not sure what update caused that, but compiler tests were failing.). (reverted this due to CI test failure. See comments below.)

Thanks.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@mattjohnsonpint mattjohnsonpint changed the title Improve date parsing (fractional seconds, UTC offsets) fix: Improve date parsing (fractional seconds, UTC offsets) Nov 18, 2023
@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Nov 18, 2023

Hmmm.. the std/math [fixture mismatch] showing on the Ubuntu compiler tests is exactly what I got locally, and why I regenerated that fixture. Perhaps it's an architectural difference? I'm running on an arm64 Mac.

I'll revert that change and see if the rest passes in CI.

@mattjohnsonpint
Copy link
Contributor Author

That seems to have been the issue. At least, reverting the fixture for std/math passed CI. I'm not sure why it's different on my machine, but I guess it's something to do with arm64. Perhaps a bug with Binaryen or another dependency? IDK. I'll open a new issue for that.

@HerrCai0907
Copy link
Member

@mattjohnsonpint Yes.please ignore std math test in aarch64. It also bother me a lot. I guess there are maybe some bugs in nodes

Copy link
Member

@CountBleck CountBleck left a comment

Choose a reason for hiding this comment

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

LGTM, since the other tests are working as intended.

@mattjohnsonpint
Copy link
Contributor Author

Applied all feedback, and rebuilt the std/date fixture on an x64 machine. The tests should pass on next run. Thanks.

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Nov 18, 2023

OK, I'm stumped. Why do I get A fixture mismatch error in CI? I used a Windows x64 machine to generate the last ones, and it still fails. Everything passes locally though. 😕

@CountBleck CountBleck requested a review from MaxGraey November 18, 2023 23:21
@mattjohnsonpint
Copy link
Contributor Author

Thanks everyone! Please merge when ready. 🙂

@CountBleck
Copy link
Member

Looks like everyone's onboard, so I'm merging this.

@CountBleck CountBleck merged commit d142ac8 into AssemblyScript:main Nov 20, 2023
@mattjohnsonpint mattjohnsonpint deleted the date-parser branch November 20, 2023 16:17
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