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

Add helpers.math._factorize #6360

Merged
merged 4 commits into from
Jul 2, 2019
Merged

Add helpers.math._factorize #6360

merged 4 commits into from
Jul 2, 2019

Conversation

benmccann
Copy link
Contributor

This is splitting off part of #6274 to make that PR smaller and easier to review. I plan to reopen that PR soon

By exporting this as part of helpers.math I'm able to add tests for this method and it also helps organize the code. The helpers.math file is also being added in #6343 in order to add a helpers.math.log10

@nagix
Copy link
Contributor

nagix commented Jun 27, 2019

I learned a bitwise OR (|) on floating-point numbers converts them to 32-bit signed integers 🙂. Wouldn't it be better to have test cases for invalid values?

@benmccann
Copy link
Contributor Author

Thanks for taking a look. I've added input validation per your suggestion

@nagix
Copy link
Contributor

nagix commented Jun 27, 2019

Thank you, but I think it would be enough to return an empty array and checking that.

@benmccann
Copy link
Contributor Author

If we return an empty array then it will be much harder to figure out why the code is not working as expected. We should never encounter a non-positive integer as input. If we do then I want to know as soon as possible so that I can find the bug causing the bad input rather than hiding the error and making it harder to find what's going wrong. Here's a detailed article about the concept: https://www.martinfowler.com/ieeeSoftware/failFast.pdf

@nagix
Copy link
Contributor

nagix commented Jun 27, 2019

That's true. However, it is an internal method and not supposed to be used by users. I feel it's too much to check and throw an error in it.

@benmccann
Copy link
Contributor Author

@etimberg @kurkle do you guys have thoughts about which is the better direction to go?

@benmccann
Copy link
Contributor Author

@nagix I had another idea. What if I return an empty array, but log a warning with console.warn? I think that would both satisfy your concern of not failing the whole chart and mine of not silently ignoring an error.

@kurkle
Copy link
Member

kurkle commented Jun 27, 2019

I do agree throw is a bit much.
You could just remove the check, Math.sqrt will return NaN on invalid input and function will return empty array. I think that's fine behavior for invalid input to internal helper.

I don't think adding console.warn is good idea either, its just bloat in the lib.

@benmccann
Copy link
Contributor Author

Ok. I've gone ahead and removed. Thanks again for taking a look at this

nagix
nagix previously approved these changes Jun 28, 2019
@benmccann benmccann requested a review from etimberg July 2, 2019 14:23
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* Add helpers.math._factorize
* Remove duplicate test statement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants