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

Use more standard representation of boolean values in cells #59

Closed
wants to merge 2 commits into from

Conversation

pineapplemachine
Copy link

@pineapplemachine pineapplemachine commented Apr 27, 2023

See https://bugs.documentfoundation.org/show_bug.cgi?id=155046

Unfortunately the OOXML ECMA-376 spec does not ever seem to explicitly mention what is expected for a <v> cell value element within a <c> cell element where t="b" indicates a boolean value type. But it seems that excel4node's writing the text "true" and "false" instead of the text "0" and "1" within <v> elements representing boolean values is uncommon behavior.

With this change, boolean cells now display as expected in LibreOffice Calc and Google Sheets, where previously they did not.

I verified on my system that npm run test and node sample.js && ./validate.sh Excel.xlsx both run successfully after building.

See https://bugs.documentfoundation.org/show_bug.cgi?id=155046

Unfortunately the OOXML ECMA-376 spec does not ever seem to explicitly mention what is expected for a `<v>` element within a `<c> element with `t="b"`. But it seems that excel4node's writing the text "true" and "false" instead of the text "0" and "1" within `<v>` elements representing boolean values is uncommon behavior.
@pineapplemachine
Copy link
Author

It occurs to me in retrospect that the intention may have been for a cell's v attribute to always be the same string as belongs in the XML - if that's the case, just let me know and I'm happy revise the PR to follow that convention.

@jrohland
Copy link

jrohland commented May 1, 2023

Thanks for the pull request! I'll hopefully get some time this week to go through it.

Unfortunately as I'm not the original developer and have recently assumed maintenance of this project I don't know the original intentions as they were. I'll take a read through and hopefully get this merged and tagged in a release soon.

@pineapplemachine
Copy link
Author

I submitted #62 which fixes the same problem but in a way that may be more consistent with how the code is structured currently. I would probably recommend merging that one instead of this one? But of course whichever you think is better - if it's possible to get to it this week in any case, that would be great.

@jrohland
Copy link

jrohland commented May 2, 2023

Closing this since it was solved in #62 and released in version 1.8.2.

Thanks for the fixes!

@jrohland jrohland closed this May 2, 2023
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.

2 participants