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

Parse Numbers as Scientific, not Double #176

Open
olafklinke opened this issue May 17, 2024 · 8 comments
Open

Parse Numbers as Scientific, not Double #176

olafklinke opened this issue May 17, 2024 · 8 comments

Comments

@olafklinke
Copy link

Currently xlsx seems to build on the Data.Text.Read module, which provide polymorphic functions like

rational :: Fractional a => Text -> Either String (a,Text)

This is however used to parse a decimal number from the XML into a Double value (note that CellDouble :: Double -> CellValue) which for most values can not represent the rational number exactly. This is especially annoying when the decimal number represents a LocalTime, because rounding errors can shift the time into a different day, thus affecting the displayed value quite a lot.
My proposed change is that

  1. CellValue gets an additional CellDecimal constructor for implementing and testing the rational parsing to Scientific.
  2. When this is established, CellDouble is deprecated.

To limit breakage, one could provide a view pattern named CellDouble that uses Data.Scientific.toRealFloat for the transition phase.

@olafklinke
Copy link
Author

Remark: I have not sanity-checked my PR, it is just the bare minimum to make the changes compile as a proof-of-concept. In particular, the TH-generated _CellDouble prism is still missing.

@qrilka
Copy link
Owner

qrilka commented May 17, 2024

Thanks @olafklinke I'll try to find time for that PR

@olafklinke
Copy link
Author

I noticed that I neglected to adjust the test suite. But a simple s/CellDouble/CellDecimal/g for each file in test/ makes all tests pass.

@qrilka
Copy link
Owner

qrilka commented May 18, 2024

Would you mind fixing the tests?

@olafklinke
Copy link
Author

Apologies, the last commit that should have fixed the test apparently did not change any files. I added a 9th commit to my fork. How do I update the PR to include these commits? Or does the PR update itself?

@qrilka
Copy link
Owner

qrilka commented Jun 1, 2024

It does but unfortunately CI run doesn't get automatically approved (probably I need to do something about that)

@olafklinke
Copy link
Author

olafklinke commented Jun 5, 2024

If the OOXML spec really states that all numbers in spreadsheets are xsd:double then the real issue is not the conversion between decimal representation and CellDouble but rather the fact that TimeOfDay and Double are in mismatch and do not properly round-trip: A second in a standard day is 1/86400 which is not a dyadic rational and hence neither representable in Double nor does it have a finite decimal expansion.

@qrilka
Copy link
Owner

qrilka commented Jun 5, 2024

Oh, shoot, I didn't check the XSD spec, thanks.
OOXML spec doesn't really state that. That was only for cached values and it seems I didn't check it all through...

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

No branches or pull requests

2 participants