Skip to content
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

Implement PlainTime #25

Merged
merged 25 commits into from
Apr 6, 2021
Merged

Conversation

ashutoshvarma
Copy link
Contributor

@ashutoshvarma ashutoshvarma commented Apr 3, 2021

Part of #21

Other fixes include in PR

@ashutoshvarma ashutoshvarma marked this pull request as draft April 3, 2021 22:36
@ashutoshvarma
Copy link
Contributor Author

From failing tests log, seems FP rounding error

[Fail]: ✖ 15:23:30.123456789.add({ nanoseconds: 300 })
[Actual]: "15:23:30.12345708899999999"
[Expected]: "15:23:30.123457089"

How should we handle floating-point rounding errors?

assembly/plaintime.ts Outdated Show resolved Hide resolved
assembly/plaintime.ts Outdated Show resolved Hide resolved
assembly/plaintime.ts Outdated Show resolved Hide resolved
@MaxGraey
Copy link
Contributor

MaxGraey commented Apr 4, 2021

How should we handle floating-point rounding errors?

Such imprecise rarely happens in dtoa implementaion in AS. I have plan rewrite that to more modern Ryu's implementation. But currently we could just ignore this imprecision and add some TODO comment

ashutoshvarma and others added 4 commits April 4, 2021 14:23
Apply suggestions from code review

Co-authored-by: Max Graey <maxgraey@gmail.com>
assembly/plaintime.ts Outdated Show resolved Hide resolved
assembly/plaintime.ts Outdated Show resolved Hide resolved
assembly/plaintime.ts Outdated Show resolved Hide resolved
Co-authored-by: Max Graey <maxgraey@gmail.com>
assembly/plaintime.ts Outdated Show resolved Hide resolved
assembly/plaintime.ts Outdated Show resolved Hide resolved
@ashutoshvarma
Copy link
Contributor Author

ashutoshvarma commented Apr 5, 2021

@MaxGraey shouldn't it be 8 instead of 4 in sign ? or am I missing something?

export function sign<T extends number>(x: T): T {
// x < 0 ? -1 : 1 -> x >> 31 | 1
// @ts-ignore
return (x >> (sizeof<T>() * 4 - 1)) | 1;
}

@MaxGraey
Copy link
Contributor

MaxGraey commented Apr 5, 2021

right! Good catch!

@ashutoshvarma
Copy link
Contributor Author

Also while working on util(), I get to know that balanceDuration is broken when largestUnit is greater than TimeComponent.days .

I will create an issue for it shortly

@ashutoshvarma
Copy link
Contributor Author

right! Good catch!

Should I add a commit for it in this PR ? PlainTime.until() tests are failing because of it too

@MaxGraey
Copy link
Contributor

MaxGraey commented Apr 5, 2021

I already made PR with fix: #28

assembly/utils.ts Outdated Show resolved Hide resolved
assembly/utils.ts Outdated Show resolved Hide resolved
@ashutoshvarma
Copy link
Contributor Author

ashutoshvarma commented Apr 5, 2021

toString(
// sort in ascending order for better sum precision
f64(this.nanoseconds) / 1000000000.0 +
f64(this.microseconds) / 1000000.0 +
f64(this.milliseconds) / 1000.0 +
f64(this.seconds),
"S"
);

@MaxGraey is there a way to remove the trailing .0 from the f64 string representation? I tried reparsing string back using parseFloat and then toString but it didn't work

From CI logs https://github.com/ColinEberhardt/assemblyscript-temporal/runs/2270205795

[Fail]: ✖ the default largest unit is at least hours
[Actual]: "PT6H52M42.0S"
[Expected]: "PT6H52M42S"

@MaxGraey
Copy link
Contributor

MaxGraey commented Apr 5, 2021

is there a way to remove the trailing .0 from the f64 string representation? I tried reparsing string back using

Trailing zero for serialized f64/f32 strings is a deliberate decision and convention of AssemblyScript which differ from JS/TS. So I suggest slightly modify original tests and add .0 when this required

@ashutoshvarma ashutoshvarma marked this pull request as ready for review April 5, 2021 15:04
@ashutoshvarma
Copy link
Contributor Author

I think this PR is good to go now!

@MaxGraey
Copy link
Contributor

MaxGraey commented Apr 5, 2021

is there a way to remove the trailing .0 from the f64 string representation? I tried reparsing string back using

Trailing zero for serialized f64/f32 strings is a deliberate decision and convention of AssemblyScript which differ from JS/TS. So I suggest slightly modify original tests and add .0 when this required

Another option:

function stringify(value: f64): string {
   return F64.isSafeInteger(value) ? i64(value).toString() : value.toString();
}

@ashutoshvarma
Copy link
Contributor Author

function stringify(value: f64): string {
   return F64.isSafeInteger(value) ? i64(value).toString() : value.toString();
}

This is great! With this Duration.toString() will be more close to tc39 implementation

@ColinEberhardt ColinEberhardt merged commit 0906295 into ColinEberhardt:master Apr 6, 2021
@ColinEberhardt
Copy link
Owner

I think this PR is good to go now!

Thank you - great work 👍

@github-actions
Copy link

github-actions bot commented Apr 6, 2021

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

balanceDuration() is broken when when largestUnit is greater than TimeComponent.days
3 participants