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

Html Reader/Writer Better Handling of Booleans #4257

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented Dec 1, 2024

When Html Writer outputs a cell with a boolean value, the result will either be 1 or null-string; neither of these is optimal for anyone looking at the resulting html. Html Reader already has the ability to recognize data types using the html data-type attribute, but Html Writer doesn't use it. This PR adds the ability to generate that attribute for booleans. It will generate a string value appropriate for the locale when it encounters a boolean. Html Reader, when it encounters data-type="b", will interpret the result as true if the value is 1 or a string value recognized as true in any locale; it will interpret the result as false if the value is 0, null-string, null, or a string value recognized as false in any locale; if none of the above, it will leave the value as an unchanged string. So, Reader will wind up with the correct result even if its locale is different than what Writer used.

Because this is a breaking change, it is opt-in. You need to call Writer::setBetterBoolean(true) in order for it take effect. The current default value for that property is false. When it is time to introduce breaking changes (see PR #4240), the default will be changed to true.

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

When Html Writer outputs a cell with a boolean value, the result will either be 1 or null-string; neither of these is optimal for anyone looking at the resulting html. Html Reader already has the ability to recognize data types using the html `data-type` attribute, but Html Writer doesn't use it. This PR adds the ability to generate that attribute for booleans. It will generate a string value appropriate for the locale when it encounters a boolean. Html Reader, when it encounters `data-type="b"`, will interpret the result as true if the value is 1 or a string value recognized as true in any locale; it will interpret the result as false if the value is 0, null-string, null, or a string value recognized as false in any locale; if none of the above, it will leave the value as an unchanged string. So, Reader will wind up with the correct result even if its locale is different than what Writer used.

Because this is a breaking change, it is opt-in. You need to call `Writer::setBetterBoolean(true)` in order for it take effect. The current default value for that property is false. When it is time to introduce breaking changes (see PR PHPOffice#4240), the default will be changed to true.
@oleibman
Copy link
Collaborator Author

oleibman commented Dec 1, 2024

No concern about Scrutinizer "complexity" message.

@oleibman oleibman added this pull request to the merge queue Dec 6, 2024
Merged via the queue into PHPOffice:master with commit e23c987 Dec 6, 2024
13 of 14 checks passed
@oleibman oleibman deleted the htmlbool branch December 6, 2024 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant