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

We need to formalize how to handle +Inf and NaN #221

Open
3 tasks
b5 opened this issue Mar 11, 2020 · 0 comments
Open
3 tasks

We need to formalize how to handle +Inf and NaN #221

b5 opened this issue Mar 11, 2020 · 0 comments

Comments

@b5
Copy link
Member

b5 commented Mar 11, 2020

qri-io/desktop#478 uncovered this with the following error message when running qri save:

validating data: error reading values: error writing row 129283: json: unsupported value: +Inf

What's going on here:

  • This is saving a CSV file, with some validation.
  • As mentioned in the issue, +Inf is being written to a cell of CSV
  • the CSV valueReader is totally fine with +Inf, but the conversion to JSON for validation is not happy with a number of type "+Inf"

Two ways this could be improved:

  • The reporting error could specify the address within the row so we know the exact cell. In the reported issue our user was concerned it might have been emojis (which were also present in that row). Specifying the row that's causing the problem would help rule this out earlier
  • Qri needs to be a little smarter about these pseudo-numeric values like NaN and +Inf. They aren't numbers, and when we encounter them, things like format conversion gets dicy.

Steps to close this

  • Write a set of good-case interop tests that push a common value of each type through a relay of all supported formats by wrapping multiple writers CSV -> XLSX -> JSON -> CBOR -> CSV. These should pass with an equality test matching input & output. This'll help confirm we're not doing silly things during format conversion
  • We need to formalize how we handle +Inf and NaN. Once we do, write a test pushing +Inf and NaN around. Let's use this issue to work that out

One Day

  • Produce a warning to lets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant