pjfanning commented on code in PR #1395:
URL: https://github.com/apache/poi/pull/1395#discussion_r4294502326
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -781,8 +781,8 @@ private String cleanFormatForNumber(String formatStrIn) {
// quoting and scientific notation
for(int i = 0; i < sb.length(); i++) {
char c = sb.charAt(i);
- // remove quotes and back slashes
- if (c == '\\' || c == '"') {
+ // remove quotes and escape characters
+ if (c == '\\' || c == '!' || c == '"') {
Review Comment:
This now removes every `!` in the format, including literal ones inside
quoted text and escaped ones (`\!`, `!!`). That is a regression. For the value
5:
- `0" Hi!"` used to give `5 Hi!` and now gives `5 Hi`
- `0\!` and `0!!` now give `5` instead of `5!`
In emulateCSV mode, a text-only format such as `"Done!"` also loses its `!`.
`!` should only be dropped when it acts as an escape character, and the
character after it should be kept.
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -799,39 +799,57 @@ private String cleanFormatForNumber(String formatStrIn) {
private static class InternalDecimalFormatWithScale extends Format {
private static final Pattern endsWithCommas = Pattern.compile("(,+)$");
- private final BigDecimal divider;
- private static final BigDecimal ONE_THOUSAND =
BigDecimal.valueOf(1000);
+ private final int powersOfTenToScaleDown;
private final DecimalFormat df;
private static String trimTrailingCommas(String s) {
return s.replaceAll(",+$", "");
}
- public InternalDecimalFormatWithScale(String pattern,
DecimalFormatSymbols symbols) {
+ private static int digitsAfterLiteralDecimalPoint(String formatStr) {
+ int digits = 0;
+ boolean afterLiteralPoint = false;
+ boolean quoted = false;
+ for (int i = 0; i < formatStr.length(); i++) {
+ char c = formatStr.charAt(i);
+ boolean escaped = (c == '\\' || c == '!') && i <
formatStr.length() - 1;
+ if (escaped) {
+ c = formatStr.charAt(++i);
+ } else if (c == '"') {
+ quoted = !quoted;
+ continue;
+ }
+ if (c == '.') {
+ afterLiteralPoint = escaped || quoted;
+ digits = 0;
+ } else if (afterLiteralPoint && (c == '0' || c == '#')) {
Review Comment:
Quoted or escaped `0`/`#` characters are counted here as digit placeholders,
but Excel treats them as literal text. With 1234567, Excel shows `123.05` for
`0".0"0,`, but this code scales by 10^5 and gives `12.35`. Only characters that
are neither quoted nor escaped should be counted.
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -799,39 +799,57 @@ private String cleanFormatForNumber(String formatStrIn) {
private static class InternalDecimalFormatWithScale extends Format {
private static final Pattern endsWithCommas = Pattern.compile("(,+)$");
- private final BigDecimal divider;
- private static final BigDecimal ONE_THOUSAND =
BigDecimal.valueOf(1000);
+ private final int powersOfTenToScaleDown;
private final DecimalFormat df;
private static String trimTrailingCommas(String s) {
return s.replaceAll(",+$", "");
}
- public InternalDecimalFormatWithScale(String pattern,
DecimalFormatSymbols symbols) {
+ private static int digitsAfterLiteralDecimalPoint(String formatStr) {
+ int digits = 0;
+ boolean afterLiteralPoint = false;
+ boolean quoted = false;
+ for (int i = 0; i < formatStr.length(); i++) {
+ char c = formatStr.charAt(i);
+ boolean escaped = (c == '\\' || c == '!') && i <
formatStr.length() - 1;
+ if (escaped) {
+ c = formatStr.charAt(++i);
+ } else if (c == '"') {
+ quoted = !quoted;
+ continue;
+ }
+ if (c == '.') {
+ afterLiteralPoint = escaped || quoted;
+ digits = 0;
+ } else if (afterLiteralPoint && (c == '0' || c == '#')) {
+ digits++;
+ }
+ }
+ return digits;
+ }
+
+ public InternalDecimalFormatWithScale(String pattern,
DecimalFormatSymbols symbols, String rawPattern) {
df = new DecimalFormat(trimTrailingCommas(pattern), symbols);
setExcelStyleRoundingMode(df);
Matcher endsWithCommasMatcher = endsWithCommas.matcher(pattern);
if (endsWithCommasMatcher.find()) {
- String commas = (endsWithCommasMatcher.group(1));
- BigDecimal temp = BigDecimal.ONE;
- for (int i = 0; i < commas.length(); ++i) {
- temp = temp.multiply(ONE_THOUSAND);
- }
- divider = temp;
+ powersOfTenToScaleDown =
Review Comment:
The extra scaling for a literal dot only happens inside the trailing-comma
branch. Without trailing commas, removing the `!` or `\` turns the literal dot
into a real decimal point, and the value is not scaled.
For `0!.0` (the format from EasyExcel #2319) with 9234, Excel shows `923.4`.
Before this PR the output was `9234.0!`; now it is `9234.0`, which is still
wrong. `0\.0` and `0"."0` behave the same way. The literal-dot adjustment
should apply whether or not there are trailing commas.
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -799,39 +799,57 @@ private String cleanFormatForNumber(String formatStrIn) {
private static class InternalDecimalFormatWithScale extends Format {
private static final Pattern endsWithCommas = Pattern.compile("(,+)$");
- private final BigDecimal divider;
- private static final BigDecimal ONE_THOUSAND =
BigDecimal.valueOf(1000);
+ private final int powersOfTenToScaleDown;
private final DecimalFormat df;
private static String trimTrailingCommas(String s) {
return s.replaceAll(",+$", "");
}
- public InternalDecimalFormatWithScale(String pattern,
DecimalFormatSymbols symbols) {
+ private static int digitsAfterLiteralDecimalPoint(String formatStr) {
+ int digits = 0;
+ boolean afterLiteralPoint = false;
+ boolean quoted = false;
+ for (int i = 0; i < formatStr.length(); i++) {
+ char c = formatStr.charAt(i);
Review Comment:
This scans the whole format string, so in a two-section format such as
`0\.00,;-0\.0,` the positive section is scaled using the digit count from the
negative section. Likewise, a real `.` in the negative section resets the
count, so the positive section loses its literal-dot adjustment. The count
should be taken per section.
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -799,39 +799,57 @@ private String cleanFormatForNumber(String formatStrIn) {
private static class InternalDecimalFormatWithScale extends Format {
private static final Pattern endsWithCommas = Pattern.compile("(,+)$");
- private final BigDecimal divider;
- private static final BigDecimal ONE_THOUSAND =
BigDecimal.valueOf(1000);
+ private final int powersOfTenToScaleDown;
private final DecimalFormat df;
private static String trimTrailingCommas(String s) {
return s.replaceAll(",+$", "");
}
- public InternalDecimalFormatWithScale(String pattern,
DecimalFormatSymbols symbols) {
+ private static int digitsAfterLiteralDecimalPoint(String formatStr) {
+ int digits = 0;
Review Comment:
This parses quotes and escapes on the raw string separately from
`cleanFormatForNumber`, and the two parsers already disagree: one ignores
`_x`/`*x` pairs and the other strips them. The disagreement is behind several
of the miscounts above. It would be simpler to record the literal-dot digit
count while `cleanFormatForNumber` strips the escapes, so a second scan isn't
needed.
##########
poi/src/test/java/org/apache/poi/ss/usermodel/TestDataFormatter.java:
##########
@@ -902,6 +902,17 @@ void testFormatWithTrailingDotsUS() {
assertEquals("1,000,000", dfUS.formatRawCellContents(1e24, -1,
"#,##0,,,,,,"));
}
+ @Test
+ void testFormatWithTrailingCommasAndFractionalValues() {
Review Comment:
These tests only use `formatRawCellContents` (the BigDecimal path). Please
also cover:
- `formatCellValue` (the double path when `requiresScaling` is true)
- `0!.0` without trailing commas (the PR description says it gives `0.9` for
9234, but the test asserts this with `0!.0,`)
- literal `!` in quoted text and `!!`, e.g. `0" Hi!"`, which would catch the
stripping regression
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -799,39 +799,57 @@ private String cleanFormatForNumber(String formatStrIn) {
private static class InternalDecimalFormatWithScale extends Format {
private static final Pattern endsWithCommas = Pattern.compile("(,+)$");
- private final BigDecimal divider;
- private static final BigDecimal ONE_THOUSAND =
BigDecimal.valueOf(1000);
+ private final int powersOfTenToScaleDown;
private final DecimalFormat df;
private static String trimTrailingCommas(String s) {
return s.replaceAll(",+$", "");
}
- public InternalDecimalFormatWithScale(String pattern,
DecimalFormatSymbols symbols) {
+ private static int digitsAfterLiteralDecimalPoint(String formatStr) {
+ int digits = 0;
+ boolean afterLiteralPoint = false;
+ boolean quoted = false;
+ for (int i = 0; i < formatStr.length(); i++) {
+ char c = formatStr.charAt(i);
+ boolean escaped = (c == '\\' || c == '!') && i <
formatStr.length() - 1;
+ if (escaped) {
+ c = formatStr.charAt(++i);
+ } else if (c == '"') {
+ quoted = !quoted;
+ continue;
+ }
+ if (c == '.') {
+ afterLiteralPoint = escaped || quoted;
+ digits = 0;
+ } else if (afterLiteralPoint && (c == '0' || c == '#')) {
+ digits++;
+ }
+ }
+ return digits;
+ }
+
+ public InternalDecimalFormatWithScale(String pattern,
DecimalFormatSymbols symbols, String rawPattern) {
df = new DecimalFormat(trimTrailingCommas(pattern), symbols);
setExcelStyleRoundingMode(df);
Matcher endsWithCommasMatcher = endsWithCommas.matcher(pattern);
if (endsWithCommasMatcher.find()) {
- String commas = (endsWithCommasMatcher.group(1));
- BigDecimal temp = BigDecimal.ONE;
- for (int i = 0; i < commas.length(); ++i) {
- temp = temp.multiply(ONE_THOUSAND);
- }
- divider = temp;
+ powersOfTenToScaleDown =
+ 3 * endsWithCommasMatcher.group(1).length() +
digitsAfterLiteralDecimalPoint(rawPattern);
} else {
Review Comment:
Scaling by an extra 10^digits and then formatting with DecimalFormat's `.`
makes a `#` after a literal dot behave like a fractional `#`, so its trailing
zero is dropped. In Excel that position is an integer digit. With 1230000,
Excel shows `123.0` for `0\.#,`, but this code gives `123`. `?` placeholders
after a literal dot are not counted at all.
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -731,7 +731,7 @@ private String cleanFormatForNumber(String formatStrIn) {
for (int i = 0; i < sb.length(); i++) {
char c = sb.charAt(i);
if (c == '_' || c == '*' || c == '?') {
Review Comment:
Any `!` before `_`/`*`/`?` is treated as escaping it, even when that `!` is
itself escaped (`\!`) or is the second character of `!!`. In `0\!_)`, Excel
shows a literal `!` followed by a space the width of `)`. Here `_)` is left in,
so the pattern passed to `DecimalFormat` is `0_)`, which gives the wrong text
or falls back to General. The emulateCSV branch has the same problem with `!!?`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
