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 Number.prototype.toPrecision #962

Merged
merged 9 commits into from
Dec 17, 2020
Merged

Implement Number.prototype.toPrecision #962

merged 9 commits into from
Dec 17, 2020

Conversation

NathanRoyer
Copy link
Contributor

This Pull Request closes #349.

It changes the following:

  • Implementation of Number.prototype.toPrecision is complete.
  • Tests of the aforementioned method were enabled.

@codecov
Copy link

codecov bot commented Dec 12, 2020

Codecov Report

Merging #962 (4146657) into master (4e82a98) will increase coverage by 0.08%.
The diff coverage is 65.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #962      +/-   ##
==========================================
+ Coverage   59.40%   59.48%   +0.08%     
==========================================
  Files         166      166              
  Lines       10717    10780      +63     
==========================================
+ Hits         6366     6413      +47     
- Misses       4351     4367      +16     
Impacted Files Coverage Δ
boa/src/builtins/number/mod.rs 63.11% <65.71%> (+2.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e82a98...f8c1abe. Read the comment docs.

@Razican Razican added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Dec 14, 2020
@Razican
Copy link
Member

Razican commented Dec 15, 2020

Test262 conformance changes:

Test result master count PR count difference
Total 78,460 78,460 0
Passed 19,374 19,386 +12
Ignored 15,559 15,559 0
Failed 43,527 43,515 -12
Panics 465 447 -18
Conformance 24.69 24.71 +0.02%

@RageKnify
Copy link
Member

The doc comment for to_precision has the wrong link to the spec, it should be https://tc39.es/ecma262/#sec-number.prototype.toprecision , can you fix it please @NathanRoyer?

@NathanRoyer
Copy link
Contributor Author

@RageKnify Done; the last commit should correct the spec and mdn links :)

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

Hey @NathanRoyer having comments with // 1, // 2, etc matching the spec's steps helps a lot in understanding the code, these are long functions and having each step identified makes it easy to check the "Rust translation", should be easy for you to do and it helps everyone who will one day read the function to understand it.
One thing I think can be changed to more closely follow the spec is in step 3 to use:

pub fn to_integer_or_infinity(&self, context: &mut Context) -> Result<IntegerOrInfinity> {

That was recently implemeted and should make the first if a bit clearer.

You can also use the following code to get precision as a Value without needing to use unwrap, you will get Value::undefined if no argument was given.

let precision = args.get(0).cloned().unwrap_or_default();

And for the links you changed the wrong ones 😅 , you changed lines 13-14 but the method's links are in lines 321-322, but don't worry just place the old links back and replaced the method's.

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

It's looking good, just a few more changes to make it easier to read it along with the spec.

boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
@Razican Razican merged commit 5c98676 into boa-dev:master Dec 17, 2020
@NathanRoyer
Copy link
Contributor Author

NathanRoyer commented Dec 17, 2020

@Razican Hey, I just found 2 bugs earlier today, and I was discussing them on the discord forum... I will include the fixes in my next contribution.

@RageKnify RageKnify added this to the v0.11.0 milestone Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Number.prototype.toPrecision
3 participants