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 decimal formatting to fix floating point to string issues #247

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mrnosal
Copy link
Contributor

@mrnosal mrnosal commented Jun 6, 2019

Fixes JS fp to string conversion issues (e.g. 94.399999999999999%)
Checks for significant digits in user entered values rather than using fixed number of decimal places.
Shows at least 1 decimal place for percentages (e.g. 3% becomes 3.0%. 0.5% means the remainder is shown as 99.5%. 0.05% gives a remainder of 99.95%, etc.)

…ring conversion issues (e.g. 94.399999999999999%)
@arscan
Copy link
Member

arscan commented Jun 7, 2019

I'm not sure why this is failing. Apparently the library you are using is packaged up as a 'module', which is something fairly new, and I'm guessing our tooling doesn't support it for one reason or another. I had hoped that this was an issue with travis not using a new enough version of node, but after upgrading node on my machine that doesn't seem to be the case.

It could have to do with the fact that this site was built using a very old create-react-app version (1.x, they are at 3.x now), and the associated react-scripts are old (which drive our test funcitonality, etc). Upgrading react-scripts does not seem like a trivial task (there are manual steps to upgrade your codebase from 1.x to 2.x, then 2.x to 3.x). I started the process but ran into issues w/xcode dependencies on my mac (recently upgraded iOS version), and since I'm getting a new lifecycle in a couple of weeks I'm not going to spend more time on this now.

Were you able to get the tests to pass on your own machine @mrnosal? Or is this a problem with the old libraries we are using as I fear?

@arscan arscan mentioned this pull request Jun 7, 2019
@arscan
Copy link
Member

arscan commented Jun 7, 2019

BTW I tried routing around this problem by doing import { Decimal } from 'decimal.js-light/decimal.js'; everywhere, but then that started causing different errors: [DecimalError] Invalid argument: [object Object]

It seems like our test environment (running in Node.js, through npm test => react-scripts test --env=jsdom) isn't getting javascript that it can execute for one reason or another. In theory this should be handled by the layers of transpilers we are using, but for some reason that isn't happening.

@mrnosal
Copy link
Contributor Author

mrnosal commented Jun 7, 2019

updating react-scripts to 1.1.5 causes .mjs to be resolved after .js files. Also fix graphviz to handle complex transitions where there is a named distribution. This is what was causing the injuries module to fail with new decimal formatting.

@dehall
Copy link
Contributor

dehall commented Jul 1, 2019

The updated formatting does look nicer but I think this needs some extra error handling.

  1. If you enter 0.0000000003 into a distributed transition, click away, then click back in, it changes into 3e-10, then if you click away again it throws an error.
  2. If you enter any invalid number (such as asdf) it throws an error, whereas previously it would just reset to 0 if someone entered an invalid number.

@mrnosal
Copy link
Contributor Author

mrnosal commented Jul 3, 2019

The issue of 0.00000003 getting converted to 3e-8 is the correct behavior for Javascript numbers. This also fails in the current code, just in a different way, since the checks for numeric strings don't accept exponential formatted numbers. It happened to work in some cases because of implicit casting of strings to numbers, but is still broke.

I can fix, but the tests for stringUtils are set to reject some strings that are valid to pass to parseFloat(). There are some slight differences between the number formats allowed by the JSON specification and the strings acceptable to parseFloat(). Since parseFloat() is more forgiving of the two, I suggest that I update stringUtils and its tests to be compatible with parseFloat.

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.

3 participants