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

Comparing string with number in BenchmarkGroup #1570

Closed
Ryuno-Ki opened this issue Dec 31, 2020 · 3 comments
Closed

Comparing string with number in BenchmarkGroup #1570

Ryuno-Ki opened this issue Dec 31, 2020 · 3 comments
Labels
bug: lite Debatable whether or not this is a bug. I mean, it’s not great but it’s not really broken either.

Comments

@Ryuno-Ki
Copy link
Contributor

Describe the bug
While adding DocStrings for type annotation, I encountered a possible bug in

if (totalForBenchmark.toFixed(0) > 0) {

Number.prototype.toFixed() is resulting a String. You are comparing it with a number.
This will likely cause JavaScript to apply a type conversion (cannot tell offset which gets converted how).
What are you trying to do here?

Expected behavior
Both parts of the comparison should have the same type.

Screenshots
typescript-check

Environment:

Additional context
Work in Progress Pull Request

@tannerdolby
Copy link
Contributor

To avoid JavaScript applying an automatic type conversion, would it be an option just to convert the result of Number.prototype.toFixed() to a number like parseInt(totalForBenchMark.toFixed(0)) within the condition of the if statement so the relational operator > can properly compare the two numbers?

@victornpb
Copy link

Number.toFixed() is mostly used for number formatting hence the return type String. So casting it back to number isn't ideal.
Since its being used as a conditional it should probably be changed to

if (Math.round(totalForBenchMark) > 0) { }
// or
if (totalForBenchMark >= 0.5) { }

@zachleat zachleat added this to the Eleventy 3.0.0 milestone Jul 10, 2024
@zachleat zachleat added bug bug: lite Debatable whether or not this is a bug. I mean, it’s not great but it’s not really broken either. and removed needs-triage bug labels Jul 10, 2024
@zachleat
Copy link
Member

Shipping with 3.0.0-alpha.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: lite Debatable whether or not this is a bug. I mean, it’s not great but it’s not really broken either.
Projects
None yet
Development

No branches or pull requests

4 participants