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

fix: JsNumberLiteralExpression::as_number ignore trivia #1447

Merged
merged 3 commits into from
Jan 6, 2024

Conversation

TheEdward162
Copy link
Contributor

Summary

JsNumberLiteralExpression::as_number wasn't ignoring trivia of the interal token, so it was almost always returning None.

Test Plan

Added a doctest, it failed without the change and now it passes.

Copy link

netlify bot commented Jan 5, 2024

Deploy Preview for biomejs canceled.

Name Link
🔨 Latest commit 95831be
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65993c8bb1970a0008825f53

@github-actions github-actions bot added A-Parser Area: parser L-JavaScript Language: JavaScript and super languages labels Jan 5, 2024
@faultyserver
Copy link
Contributor

Good catch! It looks like this also fixed an error in the snapshot test for useNumericLiterals https:/biomejs/biome/actions/runs/7427689816/job/20215337416?pr=1447#step:5:1451. If you run cargo insta review it should show you the diff between the current and the new snapshot, and you can accept the new one through that interface (by pressing a). That will update the test and make it pass here on CI.

In general, you should also use just ready when you're ready to open a PR, which will run effectively all of the CI tests on your machine before pushing so you can catch this feedback earlier and update more quickly.

@github-actions github-actions bot added the A-Linter Area: linter label Jan 6, 2024
JsNumberLiteralExpression::as_number wasn't ignoring trivia of the interal token, so it was almost always returning None
@TheEdward162
Copy link
Contributor Author

Sorry, I don't really feel comfortable installing 8 command-line tools to run the testsuite. I've updated the snapshot test using the snapshot that got emitted using cargo test. I'll leave it to CI to check if everything works now.

Also I've rebased because I accidentally used the wrong author info.

@Conaclos Conaclos merged commit ec403e8 into biomejs:main Jan 6, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants