You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fesod.apache.org by "nkuprins (via GitHub)" <gi...@apache.org> on 2026/07/20 10:35:21 UTC
[PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
nkuprins opened a new pull request, #964:
URL: https://github.com/apache/fesod/pull/964
## Purpose of the pull request
Closed: #953
Follows up on the discussion in #953. As suggested by @bengbengbalabalabeng, this encapsulates the xlsx boolean-cell parsing where it's actually used instead of exposing it as a shared utility.
## What's changed?
BooleanUtils.valueOf(String) returned true only for the exact string "1"; every other input (including "true") mapped to false. That is correct for its one job - parsing the <v> text of t="b" boolean cells, where Excel writes 0/1, matching POI's XSSFCell.getBooleanCellValue() - but as a public util named valueOf it reads like a bug and invites a future contributor to "fix" it into leniency, silently changing xlsx read behavior.
- Add CellData.setBooleanValueFromString(String) holding the "1" -> true logic (private TRUE_NUMBER constant).
- Rewire the sole caller, CellTagHandler (xlsx SAX read path), to use it. Drop the now-unused BooleanUtils import.
- Deprecate BooleanUtils.valueOf(String) - not a general-purpose string-to-boolean conversion, no other callers - pointing to the new method for future removal.
- Add CellDataTest pinning the behavior.
No behavior change: the parsing logic is identical, just relocated onto CellData. The deprecated valueOf still works for any external caller until it's removed.
## Checklist
- [x] I have read the [Contributor Guide](https://fesod.apache.org/community/contribution/).
- [ ] I have written the necessary doc or comment.
- [x] I have added the necessary unit tests and all cases have passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on PR #964:
URL: https://github.com/apache/fesod/pull/964#issuecomment-5068128902
There are some conflicts in the current branch. Please resolve them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "nkuprins (via GitHub)" <gi...@apache.org>.
nkuprins commented on code in PR #964:
URL: https://github.com/apache/fesod/pull/964#discussion_r3647707529
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/data/CellData.java:
##########
@@ -71,6 +76,19 @@ public class CellData<T> extends AbstractCell {
*/
private FormulaData formulaData;
+ /**
+ * Sets {@link #booleanValue} from the raw {@code <v>} text of a boolean ({@code t="b"}) xlsx cell.
+ *
+ * <p>Excel encodes boolean cells using the numeric markers {@code 0}/{@code 1}, so only the exact
+ * string {@code "1"} maps to {@code true}; any other input (including {@code "true"}) maps to
+ * {@code false}. This mirrors Apache POI's {@code XSSFCell.getBooleanCellValue()}.
+ *
+ * @param str the raw cell value text
+ */
+ public void setBooleanValueFromString(String str) {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "delei (via GitHub)" <gi...@apache.org>.
delei merged PR #964:
URL: https://github.com/apache/fesod/pull/964
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "delei (via GitHub)" <gi...@apache.org>.
delei commented on code in PR #964:
URL: https://github.com/apache/fesod/pull/964#discussion_r3645841739
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/data/CellData.java:
##########
@@ -71,6 +76,19 @@ public class CellData<T> extends AbstractCell {
*/
private FormulaData formulaData;
+ /**
+ * Sets {@link #booleanValue} from the raw {@code <v>} text of a boolean ({@code t="b"}) xlsx cell.
+ *
+ * <p>Excel encodes boolean cells using the numeric markers {@code 0}/{@code 1}, so only the exact
+ * string {@code "1"} maps to {@code true}; any other input (including {@code "true"}) maps to
+ * {@code false}. This mirrors Apache POI's {@code XSSFCell.getBooleanCellValue()}.
+ *
+ * @param str the raw cell value text
+ */
+ public void setBooleanValueFromString(String str) {
Review Comment:
There is no logical issue.
FYI, I will just add a point from the design perspective. I have noticed that the CellData class is a highly abstract entity. Is it more appropriate for the assignment of internal attributes to be handled in the subclasses `ReadCellData` and `WriteCellData` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "delei (via GitHub)" <gi...@apache.org>.
delei commented on code in PR #964:
URL: https://github.com/apache/fesod/pull/964#discussion_r3646422874
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/data/CellData.java:
##########
@@ -71,6 +76,19 @@ public class CellData<T> extends AbstractCell {
*/
private FormulaData formulaData;
+ /**
+ * Sets {@link #booleanValue} from the raw {@code <v>} text of a boolean ({@code t="b"}) xlsx cell.
+ *
+ * <p>Excel encodes boolean cells using the numeric markers {@code 0}/{@code 1}, so only the exact
+ * string {@code "1"} maps to {@code true}; any other input (including {@code "true"}) maps to
+ * {@code false}. This mirrors Apache POI's {@code XSSFCell.getBooleanCellValue()}.
+ *
+ * @param str the raw cell value text
+ */
+ public void setBooleanValueFromString(String str) {
Review Comment:
SGTM. You can continue to commit or update this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "nkuprins (via GitHub)" <gi...@apache.org>.
nkuprins commented on code in PR #964:
URL: https://github.com/apache/fesod/pull/964#discussion_r3646037205
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/data/CellData.java:
##########
@@ -71,6 +76,19 @@ public class CellData<T> extends AbstractCell {
*/
private FormulaData formulaData;
+ /**
+ * Sets {@link #booleanValue} from the raw {@code <v>} text of a boolean ({@code t="b"}) xlsx cell.
+ *
+ * <p>Excel encodes boolean cells using the numeric markers {@code 0}/{@code 1}, so only the exact
+ * string {@code "1"} maps to {@code true}; any other input (including {@code "true"}) maps to
+ * {@code false}. This mirrors Apache POI's {@code XSSFCell.getBooleanCellValue()}.
+ *
+ * @param str the raw cell value text
+ */
+ public void setBooleanValueFromString(String str) {
Review Comment:
Good point, agreed. Should I then move `setBooleanValueFromString` (and its `TRUE_NUMBER` constant) down to `ReadCellData` and commit-push?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "nkuprins (via GitHub)" <gi...@apache.org>.
nkuprins commented on PR #964:
URL: https://github.com/apache/fesod/pull/964#issuecomment-5068332544
> There are some conflicts in the current branch. Please resolve them.
Done. I can also add you as a _co-author_ to the commit if you'd like, given your suggestions on the implementation :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: encapsulate xlsx boolean cell parsing in CellData [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on PR #964:
URL: https://github.com/apache/fesod/pull/964#issuecomment-5069757396
> Done. I can also add you as a _co-author_ to the commit if you'd like, given your suggestions on the implementation :)
Thanks for the co-author offer. I appreciate it, but the implementation is your work, so no need to add me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org