codeant-ai-for-open-source[bot] commented on code in PR #41780:
URL: https://github.com/apache/superset/pull/41780#discussion_r3566645892
##########
superset/views/core.py:
##########
@@ -129,6 +130,11 @@
SqlResults = dict[str, Any]
+# Matches the locale codes Superset actually ships: a 2-3 letter language
+# subtag, optionally followed by a 2-letter region subtag ("pt_BR") or a
+# 4-letter script subtag ("sr_Latn").
+LANGUAGE_CODE_RE = re.compile(r"^[a-z]{2,3}(_[A-Z]{2}|_[A-Z][a-z]{3})?$")
Review Comment:
**Suggestion:** Add an explicit type annotation for the compiled regex
constant to satisfy the requirement for annotating relevant new variables.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new module-level constant is a relevant variable that can be explicitly
annotated (for example, as a compiled pattern type), and it currently has no
type hint, matching the custom rule for required Python type hints.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4dc7810211384467873b9e235d769d56&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4dc7810211384467873b9e235d769d56&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/core.py
**Line:** 136:136
**Comment:**
*Custom Rule: Add an explicit type annotation for the compiled regex
constant to satisfy the requirement for annotating relevant new variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=5b314f8ca4fcf1fbfed2272e1167845d6ca7a0989207254ecab9c70a3fe4c428&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=5b314f8ca4fcf1fbfed2272e1167845d6ca7a0989207254ecab9c70a3fe4c428&reaction=dislike'>๐</a>
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -156,71 +156,106 @@ def test_bootstrap_does_not_crash_without_recaptcha_key(
assert "RECAPTCHA_PUBLIC_KEY" not in payload["conf"]
-# --- language_pack injection --------------------------------------------
+# --- language pack delivery ----------------------------------------------
#
-# The Jed pack is injected by `common_bootstrap_payload` (outside the
-# memoized `cached_common_bootstrap_data`) using the shared
-# `superset.translations.utils.get_language_pack`. Tests here cover the
-# wrapper to confirm the pack lands on the payload for non-English
-# locales and is None for English.
+# The pack is NOT embedded in the bootstrap payload; spa.html loads it via a
+# content-addressed script tag whose URL comes from
+# `get_language_pack_template_context`. A pack supplied through
+# COMMON_BOOTSTRAP_OVERRIDES_FUNC still rides the payload and suppresses the
+# script tag.
-def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+def test_common_bootstrap_payload_does_not_embed_language_pack(
app_context: None,
) -> None:
- """common.language_pack carries the shared utility's pack for non-en."""
- fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ """The payload stays small: no full pack even for non-English locales."""
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value={"locale": "fr"},
),
- patch(
- "superset.views.base.get_language_pack",
- return_value=fake_pack,
- ) as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] == fake_pack
- mock_get.assert_called_once_with("fr")
+ assert payload["language_pack"] is None
-def test_common_bootstrap_payload_skips_pack_for_english(
+def test_common_bootstrap_payload_preserves_override_pack(
app_context: None,
) -> None:
- """English short-circuits: pack is None and the utility is not called."""
+ """A pack from COMMON_BOOTSTRAP_OVERRIDES_FUNC is passed through."""
+ fake_pack: dict[str, Any] = {"domain": "superset", "locale_data":
{"superset": {}}}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
- return_value={"locale": "en"},
+ return_value={"locale": "fr", "language_pack": fake_pack},
),
- patch("superset.views.base.get_language_pack") as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
- patch("superset.views.base.get_locale", return_value="en"),
+ patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] is None
- mock_get.assert_not_called()
+ assert payload["language_pack"] == fake_pack
def test_common_bootstrap_payload_does_not_mutate_memoized_dict(
app_context: None,
) -> None:
- """Injecting language_pack must not write back into the memoize cache."""
+ """Defaulting language_pack must not write back into the memoize cache."""
cached: dict[str, Any] = {"locale": "fr"}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value=cached,
),
- patch("superset.views.base.get_language_pack", return_value={"x": 1}),
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
common_bootstrap_payload()
assert "language_pack" not in cached
+
+
+def _language_pack_context(
+ locale: str, payload_extra: dict[str, Any]
+) -> dict[str, Any]:
+ from superset.views.base import get_language_pack_template_context
+
+ with (
+ patch(
+ "superset.views.base.get_language_pack_version",
+ return_value="abc123def456",
+ ),
+ patch(
+ "superset.views.base.url_for",
+ return_value="/language_pack/fr/abc123def456/script.js",
+ ),
+ ):
+ return get_language_pack_template_context({"locale": locale,
**payload_extra})
+
+
+def test_language_pack_template_context_versioned_src_for_non_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("fr", {})
Review Comment:
**Suggestion:** Add an explicit type annotation to the local `context`
variable to satisfy the type-hint requirement for annotatable variables.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is new Python code introducing a local variable with an inferable
structured type, and it is not annotated. That matches the rule requiring type
hints for annotatable variables.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2ec958fc027d40b6bebb239f308d07f1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2ec958fc027d40b6bebb239f308d07f1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_bootstrap_auth.py
**Line:** 242:242
**Comment:**
*Custom Rule: Add an explicit type annotation to the local `context`
variable to satisfy the type-hint requirement for annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=bfc1eec5c8c5cc8bd1f4316a06dace8ab59c6539d6e18bad6bc4fbcc1c74d86b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=bfc1eec5c8c5cc8bd1f4316a06dace8ab59c6539d6e18bad6bc4fbcc1c74d86b&reaction=dislike'>๐</a>
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -156,71 +156,106 @@ def test_bootstrap_does_not_crash_without_recaptcha_key(
assert "RECAPTCHA_PUBLIC_KEY" not in payload["conf"]
-# --- language_pack injection --------------------------------------------
+# --- language pack delivery ----------------------------------------------
#
-# The Jed pack is injected by `common_bootstrap_payload` (outside the
-# memoized `cached_common_bootstrap_data`) using the shared
-# `superset.translations.utils.get_language_pack`. Tests here cover the
-# wrapper to confirm the pack lands on the payload for non-English
-# locales and is None for English.
+# The pack is NOT embedded in the bootstrap payload; spa.html loads it via a
+# content-addressed script tag whose URL comes from
+# `get_language_pack_template_context`. A pack supplied through
+# COMMON_BOOTSTRAP_OVERRIDES_FUNC still rides the payload and suppresses the
+# script tag.
-def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+def test_common_bootstrap_payload_does_not_embed_language_pack(
app_context: None,
) -> None:
- """common.language_pack carries the shared utility's pack for non-en."""
- fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ """The payload stays small: no full pack even for non-English locales."""
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value={"locale": "fr"},
),
- patch(
- "superset.views.base.get_language_pack",
- return_value=fake_pack,
- ) as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] == fake_pack
- mock_get.assert_called_once_with("fr")
+ assert payload["language_pack"] is None
-def test_common_bootstrap_payload_skips_pack_for_english(
+def test_common_bootstrap_payload_preserves_override_pack(
app_context: None,
) -> None:
- """English short-circuits: pack is None and the utility is not called."""
+ """A pack from COMMON_BOOTSTRAP_OVERRIDES_FUNC is passed through."""
+ fake_pack: dict[str, Any] = {"domain": "superset", "locale_data":
{"superset": {}}}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
- return_value={"locale": "en"},
+ return_value={"locale": "fr", "language_pack": fake_pack},
),
- patch("superset.views.base.get_language_pack") as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
- patch("superset.views.base.get_locale", return_value="en"),
+ patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] is None
- mock_get.assert_not_called()
+ assert payload["language_pack"] == fake_pack
def test_common_bootstrap_payload_does_not_mutate_memoized_dict(
app_context: None,
) -> None:
- """Injecting language_pack must not write back into the memoize cache."""
+ """Defaulting language_pack must not write back into the memoize cache."""
cached: dict[str, Any] = {"locale": "fr"}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value=cached,
),
- patch("superset.views.base.get_language_pack", return_value={"x": 1}),
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
common_bootstrap_payload()
assert "language_pack" not in cached
+
+
+def _language_pack_context(
+ locale: str, payload_extra: dict[str, Any]
+) -> dict[str, Any]:
+ from superset.views.base import get_language_pack_template_context
+
+ with (
+ patch(
+ "superset.views.base.get_language_pack_version",
+ return_value="abc123def456",
+ ),
+ patch(
+ "superset.views.base.url_for",
+ return_value="/language_pack/fr/abc123def456/script.js",
+ ),
+ ):
+ return get_language_pack_template_context({"locale": locale,
**payload_extra})
+
+
+def test_language_pack_template_context_versioned_src_for_non_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("fr", {})
+ assert context == {
+ "language_pack_src": "/language_pack/fr/abc123def456/script.js",
+ "language_pack_inline": False,
+ }
+
+
+def test_language_pack_template_context_none_for_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("en", {})
Review Comment:
**Suggestion:** Add an explicit type annotation to the local `context`
variable in this test as well. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is another newly introduced local variable in modified Python code
without a type annotation, so it violates the type-hint requirement for
annotatable variables.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9a54979cc86342bb88e5254cd29035fa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9a54979cc86342bb88e5254cd29035fa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_bootstrap_auth.py
**Line:** 252:252
**Comment:**
*Custom Rule: Add an explicit type annotation to the local `context`
variable in this test as well.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=849265d7a48093df22f1926fafd8074df11d17cd9e2b86a81fa7d12a0f25e7d3&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=849265d7a48093df22f1926fafd8074df11d17cd9e2b86a81fa7d12a0f25e7d3&reaction=dislike'>๐</a>
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -156,71 +156,106 @@ def test_bootstrap_does_not_crash_without_recaptcha_key(
assert "RECAPTCHA_PUBLIC_KEY" not in payload["conf"]
-# --- language_pack injection --------------------------------------------
+# --- language pack delivery ----------------------------------------------
#
-# The Jed pack is injected by `common_bootstrap_payload` (outside the
-# memoized `cached_common_bootstrap_data`) using the shared
-# `superset.translations.utils.get_language_pack`. Tests here cover the
-# wrapper to confirm the pack lands on the payload for non-English
-# locales and is None for English.
+# The pack is NOT embedded in the bootstrap payload; spa.html loads it via a
+# content-addressed script tag whose URL comes from
+# `get_language_pack_template_context`. A pack supplied through
+# COMMON_BOOTSTRAP_OVERRIDES_FUNC still rides the payload and suppresses the
+# script tag.
-def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+def test_common_bootstrap_payload_does_not_embed_language_pack(
app_context: None,
) -> None:
- """common.language_pack carries the shared utility's pack for non-en."""
- fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ """The payload stays small: no full pack even for non-English locales."""
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value={"locale": "fr"},
),
- patch(
- "superset.views.base.get_language_pack",
- return_value=fake_pack,
- ) as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] == fake_pack
- mock_get.assert_called_once_with("fr")
+ assert payload["language_pack"] is None
-def test_common_bootstrap_payload_skips_pack_for_english(
+def test_common_bootstrap_payload_preserves_override_pack(
app_context: None,
) -> None:
- """English short-circuits: pack is None and the utility is not called."""
+ """A pack from COMMON_BOOTSTRAP_OVERRIDES_FUNC is passed through."""
+ fake_pack: dict[str, Any] = {"domain": "superset", "locale_data":
{"superset": {}}}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
- return_value={"locale": "en"},
+ return_value={"locale": "fr", "language_pack": fake_pack},
),
- patch("superset.views.base.get_language_pack") as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
- patch("superset.views.base.get_locale", return_value="en"),
+ patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] is None
- mock_get.assert_not_called()
+ assert payload["language_pack"] == fake_pack
def test_common_bootstrap_payload_does_not_mutate_memoized_dict(
app_context: None,
) -> None:
- """Injecting language_pack must not write back into the memoize cache."""
+ """Defaulting language_pack must not write back into the memoize cache."""
cached: dict[str, Any] = {"locale": "fr"}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value=cached,
),
- patch("superset.views.base.get_language_pack", return_value={"x": 1}),
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
common_bootstrap_payload()
assert "language_pack" not in cached
+
+
+def _language_pack_context(
+ locale: str, payload_extra: dict[str, Any]
+) -> dict[str, Any]:
+ from superset.views.base import get_language_pack_template_context
+
+ with (
+ patch(
+ "superset.views.base.get_language_pack_version",
+ return_value="abc123def456",
+ ),
+ patch(
+ "superset.views.base.url_for",
+ return_value="/language_pack/fr/abc123def456/script.js",
+ ),
+ ):
+ return get_language_pack_template_context({"locale": locale,
**payload_extra})
+
+
+def test_language_pack_template_context_versioned_src_for_non_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("fr", {})
+ assert context == {
+ "language_pack_src": "/language_pack/fr/abc123def456/script.js",
+ "language_pack_inline": False,
+ }
+
+
+def test_language_pack_template_context_none_for_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("en", {})
+ assert context == {"language_pack_src": None, "language_pack_inline":
False}
+
+
+def test_language_pack_template_context_inline_for_override_pack(
+ app_context: None,
+) -> None:
+ """An operator-supplied pack suppresses the script tag; spa.html
inlines."""
+ context = _language_pack_context("fr", {"language_pack": {"domain":
"superset"}})
Review Comment:
**Suggestion:** Add a type annotation for the `context` variable to align
with enforced type-hint coverage for new Python code. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This newly added local variable is also left unannotated despite being a
clear candidate for a type hint, so the rule applies here as well.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b1f7cb4a8c94cfea02ef533e9659825&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4b1f7cb4a8c94cfea02ef533e9659825&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_bootstrap_auth.py
**Line:** 260:260
**Comment:**
*Custom Rule: Add a type annotation for the `context` variable to align
with enforced type-hint coverage for new Python code.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=596035c50185aa154a11fade6ba992fb065e1abda053595a0e9d29f0fcb0cd64&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=596035c50185aa154a11fade6ba992fb065e1abda053595a0e9d29f0fcb0cd64&reaction=dislike'>๐</a>
--
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]
