-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
Bug: Strings in Excel number fomat do not preserve case(fixes #6317) #63124
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
68b3e83
5a2f9fc
3d1abe7
daf33a9
e38dba5
a1ec4fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,22 @@ | |
| ) | ||
|
|
||
|
|
||
| def _normalize_number_format_value(value: str) -> str: | ||
| value = value.strip() | ||
| out: list[str] = [] | ||
| in_string = False | ||
|
|
||
| for ch in value: | ||
| if ch == '"': | ||
| out.append(ch) | ||
| in_string = not in_string | ||
| elif in_string: | ||
| out.append(ch) # preserve case inside string literals | ||
| else: | ||
| out.append(ch.lower()) # normalize outside literals | ||
| return "".join(out) | ||
|
|
||
|
|
||
| def _side_expander(prop_fmt: str) -> Callable: | ||
| """ | ||
| Wrapper to expand shorthand property into top, right, bottom, left properties | ||
|
|
@@ -391,7 +407,10 @@ def _error() -> str: | |
| def atomize(self, declarations: Iterable) -> Generator[tuple[str, str]]: | ||
| for prop, value in declarations: | ||
| prop = prop.lower() | ||
| value = value.lower() | ||
| if prop == "number-format": | ||
| value = _normalize_number_format_value(value) | ||
| else: | ||
| value = value.lower() | ||
| if prop in self.CSS_EXPANSIONS: | ||
| expand = self.CSS_EXPANSIONS[prop] | ||
| yield from expand(self, prop, value) | ||
|
|
@@ -414,7 +433,10 @@ def parse(self, declarations_str: str) -> Iterator[tuple[str, str]]: | |
| prop, sep, val = decl.partition(":") | ||
| prop = prop.strip().lower() | ||
| # TODO: don't lowercase case sensitive parts of values (strings) | ||
| val = val.strip().lower() | ||
| if prop == "number-format": | ||
| val = _normalize_number_format_value(val) | ||
| else: | ||
| val = val.strip().lower() | ||
|
Comment on lines
438
to
+442
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One question I have is why are we doing
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We lowercase CSS properties and values because CSS is defined as case-insensitive for all keywords and the entire Styler formatting engine is built on that assumption. If we remove .lower(), we immediately break core parts of the parser because the implementation expects all tokens to be normalized before comparison. Everything from font weights, border styles, colors and unit names is matched against internal tables that contain only lowercase entries. Without .lower(), inputs like "BOLD", "SOLID", "Red", "PX", "None", or "THIN" stop matching and fall through the logic. |
||
| if sep: | ||
| yield prop, val | ||
| else: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is not reliable, quotes can also be single quotes no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the changes as requested. Although I don't think excel supports single-quoted literals in number formats.