Copilot commented on code in PR #39712:
URL: https://github.com/apache/superset/pull/39712#discussion_r3236298512
##########
tests/unit_tests/result_set_test.py:
##########
@@ -146,6 +147,49 @@ def test_stringify_with_null_timestamps():
assert np.array_equal(result_set, expected)
[email protected](
+ ("nested_value", "expected"),
+ [
+ pytest.param(
+ ["ASCII", "plain text"],
+ '["ASCII", "plain text"]',
+ id="ascii",
+ ),
+ pytest.param(
+ ["日本語", "ひらがな"],
+ '["日本語", "ひらがな"]',
+ id="japanese",
+ ),
+ pytest.param(
+ ["móre", "áccent"],
+ '["móre", "áccent"]',
+ id="accented-latin",
+ ),
+ pytest.param(
+ ["emoji", "😁"],
+ '["emoji", "😁"]',
+ id="emoji",
+ ),
+ ],
+)
Review Comment:
This test asserts exact JSON string formatting (including spaces after
commas). That makes it brittle if encoder defaults change (e.g.,
separators/pretty settings) while still preserving Unicode correctly. Consider
asserting properties tied to the behavior under test (e.g., the string contains
the literal Unicode characters and does not contain `\\u` escapes), or parsing
the JSON and asserting semantic equality while separately asserting that the
raw string includes the expected Unicode glyphs.
##########
superset/utils/json.py:
##########
@@ -194,6 +194,7 @@ def dumps( # pylint: disable=too-many-arguments
separators: Union[tuple[str, str], None] = None,
cls: Union[type[simplejson.JSONEncoder], None] = None,
encoding: Optional[str] = "utf-8",
+ ensure_ascii: bool = True,
) -> str:
Review Comment:
The docstring for `dumps` documents several parameters but does not mention
the newly added `ensure_ascii` option (and `encoding` is also currently
undocumented). Please update the docstring to include `:param ensure_ascii:`
(and ideally `:param encoding:`) so callers understand how Unicode escaping is
controlled and what the default behavior is.
##########
tests/unit_tests/result_set_test.py:
##########
@@ -146,6 +147,49 @@ def test_stringify_with_null_timestamps():
assert np.array_equal(result_set, expected)
[email protected](
+ ("nested_value", "expected"),
+ [
+ pytest.param(
+ ["ASCII", "plain text"],
+ '["ASCII", "plain text"]',
+ id="ascii",
+ ),
+ pytest.param(
+ ["日本語", "ひらがな"],
+ '["日本語", "ひらがな"]',
+ id="japanese",
+ ),
+ pytest.param(
+ ["móre", "áccent"],
+ '["móre", "áccent"]',
+ id="accented-latin",
+ ),
+ pytest.param(
+ ["emoji", "😁"],
+ '["emoji", "😁"]',
+ id="emoji",
+ ),
+ ],
+)
+def test_stringify_nested_values_preserves_unicode(
+ nested_value: list[str], expected: str
+) -> None:
+ """
+ Nested values should be stringified without escaping Unicode characters.
+ """
+
+ data = [(nested_value,)]
+ description: DbapiDescription = [
+ ("tags", "ARRAY<STRING>", None, None, None, None, False)
+ ]
+
+ result_set = SupersetResultSet(data, description, BaseEngineSpec)
+ df = result_set.to_pandas_df()
+
+ assert df["tags"].iloc[0] == expected
Review Comment:
This test asserts exact JSON string formatting (including spaces after
commas). That makes it brittle if encoder defaults change (e.g.,
separators/pretty settings) while still preserving Unicode correctly. Consider
asserting properties tied to the behavior under test (e.g., the string contains
the literal Unicode characters and does not contain `\\u` escapes), or parsing
the JSON and asserting semantic equality while separately asserting that the
raw string includes the expected Unicode glyphs.
--
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]
