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

Feature/number object #182

Merged
merged 5 commits into from Nov 6, 2019
Merged

Conversation

pop
Copy link
Contributor

@pop pop commented Oct 22, 2019

Hey @jasonwilliams. This isn't quite done yet but I would love to get your feedback.

I have most of Number() working (I think) with a few exceptions:

  • toPrecision() is not done. It's a tough function to write and I haven't figured out how to write it.
  • toLocaleString() is not great.

We also have a problem where trying to parse something like Number(0.00) and Number(1e+5) fail. I think this is beyond the scope of the project. My workaround is to parse those cases as strings: Number("1e+5") works as expected.

Let me know if I should make changes to my tests or implementations, or if you have thoughts about the toLocaleString and toPrecision functions.

@pop pop force-pushed the feature/number-object branch 2 times, most recently from 6d82f34 to 895b974 Compare October 22, 2019 04:15
src/lib/js/number.rs Outdated Show resolved Hide resolved
@jasonwilliams
Copy link
Member

Apart from that this is looking good, @pop
you do have some conflicts now and breaking tests

@pop pop force-pushed the feature/number-object branch 3 times, most recently from 12d81c9 to 439491a Compare October 27, 2019 19:09
@jasonwilliams
Copy link
Member

I will play with toPrecision() and im not sure what we do about locale at this point

@jasonwilliams
Copy link
Member

We also have a problem where trying to parse something like Number(0.00) and Number(1e+5) fail. I think this is beyond the scope of the project. My workaround is to parse those cases as strings: Number("1e+5") works as expected.

That should have been handled here:
https://github.com/jasonwilliams/boa/blob/master/src/lib/syntax/lexer.rs#L948-L964

@pop
Copy link
Contributor Author

pop commented Oct 28, 2019

@jasonwilliams Strange. I'll try to play around with it more. I may be consuming numbers incorrectly.

@pop
Copy link
Contributor Author

pop commented Nov 2, 2019

I did some tests and I think the parsing for the scientific notation is a bit off:

Number(3e12) // Passes
Number(-3e-12) // Fails
Number(3e-12) // Passes
Number(-3e-12) // Fails
Number(3.34e12) // Fails
Number(3.45e-12) // Fails
Number(-3.45e-12) // Fails

All of these work in Node.js and there they parse to the floating point number we would expect.

The failures in boa all look like this:

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/boa`
thread 'main' panicked at 'parsing failed: Expected([Punctuator(Comma), Punctuator(CloseParen)], Token { data: Identifier("e"), pos: Position { column_number: 19, line_number: 1 } }, "function call arguments")', src/libcore/result.rs:1084:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

The tests you linked to only include scientific notation numbers of the form <Positive Int>e<Int>, so I suspect this was just an implementation oversight.

I still think that fixing this is out of scope for this issue. Thoughts? I'd love to see this merged, but I'm willing to make more changes 😅

@jasonwilliams
Copy link
Member

Just checked out your branch

let a = Number(-3e12);
a;

seems to work fine for me

@jasonwilliams
Copy link
Member

I think the best situation would be to make an issue with the scientific notation that fails and we can fix those there.

Includes:
- make_number()
- call_number()
- Number().toExponential()
- Number().toFixed()
- Number().toLocaleString() (ish)
- Number().toString()
- Number().valueOf()
- create_constructor()
- init()

Missing:
- Number().toPrecision()

refs boa-dev#34
I don't have all the context on _why_ but all of the tests for `Number` started failing.

Upon investigation it was becuase `const` stopped acting the way I expected.
Switching my test cases from using `const` to using `var` variable declarations
made everything work again.  I don't know enough about JS to know if this is a
bug or expected behavior.  Based on changes to other tests, it is know
behavior.

Refs boa-dev#34
Includes some clippy fixes.

Fixes boa-dev#34
@pop
Copy link
Contributor Author

pop commented Nov 5, 2019

@jasonwilliams I narrowed it down to floats in scientific notation not parsing correctly, so my -3e12 was just wrong 😓 sorry about that. They parse to undefined on their own and cause a panic when parsed in Number() which is not how Node.js behaves. Not sure if Node is written to spec, but it still seems outside the scope of this PR.

@jasonwilliams
Copy link
Member

This looks good, im glad you raised an issue we can fix that separately.
Great work!

@jasonwilliams jasonwilliams merged commit d725b0c into boa-dev:master Nov 6, 2019
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.

None yet

2 participants