korbit-ai[bot] commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2243963122
##########
superset/thumbnails/digest.py:
##########
@@ -91,17 +91,17 @@ def _adjust_string_with_rls(
def get_dashboard_digest(dashboard: Dashboard) -> str | None:
- config = current_app.config
+ conf = current_app.config
try:
executor_type, executor = get_executor(
- executors=config["THUMBNAIL_EXECUTORS"],
+ executors=conf["THUMBNAIL_EXECUTORS"],
model=dashboard,
current_user=get_current_user(),
)
except ExecutorNotFoundError:
return None
- if func := config["THUMBNAIL_DASHBOARD_DIGEST_FUNC"]:
+ if func := conf["THUMBNAIL_DASHBOARD_DIGEST_FUNC"]:
return func(dashboard, executor_type, executor)
unique_string = (
Review Comment:
### Duplicate Digest Generation Structure <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The get_dashboard_digest and get_chart_digest functions share significant
structural duplication in their executor retrieval and configuration handling
logic.
###### Why this matters
Violates the DRY principle, making maintenance more difficult and increasing
the risk of inconsistencies when changes are needed. Future modifications would
require updates in multiple places.
###### Suggested change ∙ *Feature Preview*
Extract the common logic into a base function that handles executor
retrieval and configuration checking:
```python
def _get_digest_base(model, config_key, digest_func):
conf = current_app.config
try:
executor_type, executor = get_executor(
executors=conf["THUMBNAIL_EXECUTORS"],
model=model,
current_user=get_current_user(),
)
except ExecutorNotFoundError:
return None
if func := conf[config_key]:
return func(model, executor_type, executor)
return digest_func(model, executor_type, executor)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0debaff-bb1f-4e6a-aa0e-da5fcf0264d7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0debaff-bb1f-4e6a-aa0e-da5fcf0264d7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0debaff-bb1f-4e6a-aa0e-da5fcf0264d7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0debaff-bb1f-4e6a-aa0e-da5fcf0264d7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0debaff-bb1f-4e6a-aa0e-da5fcf0264d7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e14c4941-8c59-4e25-afb2-e8ea2beabb29 -->
[](e14c4941-8c59-4e25-afb2-e8ea2beabb29)
##########
superset/cli/main.py:
##########
@@ -22,28 +22,46 @@
import click
from colorama import Fore, Style
+from flask import current_app
from flask.cli import FlaskGroup, with_appcontext
-from superset import app, appbuilder, cli, security_manager
-from superset.cli.lib import normalize_token
+from superset import appbuilder, cli, security_manager
from superset.extensions import db
from superset.utils.decorators import transaction
logger = logging.getLogger(__name__)
+def normalize_token(token_name: str) -> str:
+ """
+ As of click>=7, underscores in function names are replaced by dashes.
+ To avoid the need to rename all cli functions, e.g. load_examples to
+ load-examples, this function is used to convert dashes back to
+ underscores.
+
+ :param token_name: token name possibly containing dashes
+ :return: token name where dashes are replaced with underscores
+ """
+ return token_name.replace("_", "-")
+
+
+def create_app() -> Any:
+ """Create app instance for CLI"""
Review Comment:
### Insufficient context in create_app docstring <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The docstring is too terse and doesn't explain why a separate app creation
function is needed for CLI.
###### Why this matters
The context of why this function exists separately is important for
maintainers to understand the design decision.
###### Suggested change ∙ *Feature Preview*
"""Create a Flask app instance specifically for CLI usage to ensure proper
initialization of CLI commands."""
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/49e73778-a7a7-4ff9-a683-c0c88a2c640d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/49e73778-a7a7-4ff9-a683-c0c88a2c640d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/49e73778-a7a7-4ff9-a683-c0c88a2c640d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/49e73778-a7a7-4ff9-a683-c0c88a2c640d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/49e73778-a7a7-4ff9-a683-c0c88a2c640d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4ae0f05b-566f-4306-aaa8-e2b899d39449 -->
[](4ae0f05b-566f-4306-aaa8-e2b899d39449)
##########
superset/views/error_handling.py:
##########
@@ -50,6 +51,7 @@
if typing.TYPE_CHECKING:
from superset.views.base import BaseSupersetView
+conf = current_app.config
Review Comment:
### Premature current_app access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Accessing current_app.config outside of application context can cause
runtime errors.
###### Why this matters
The global assignment of current_app.config will fail with RuntimeError:
Working outside of application context when the module is imported, as
current_app is only available within a Flask application context.
###### Suggested change ∙ *Feature Preview*
Move the configuration access inside the functions where it's needed:
```python
# Remove the global assignment
# conf = current_app.config
# Instead, use current_app.config directly in the functions:
def show_http_exception(ex: HTTPException) -> FlaskResponse:
if (
"text/html" in request.accept_mimetypes
and not current_app.config["DEBUG"]
and ex.code in {404, 500}
):
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/301a6749-193c-41d6-801f-096e5705a2e1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/301a6749-193c-41d6-801f-096e5705a2e1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/301a6749-193c-41d6-801f-096e5705a2e1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/301a6749-193c-41d6-801f-096e5705a2e1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/301a6749-193c-41d6-801f-096e5705a2e1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9e0598ea-ee86-4755-b687-e03141fe8e36 -->
[](9e0598ea-ee86-4755-b687-e03141fe8e36)
##########
superset/datasource/api.py:
##########
@@ -114,7 +115,8 @@ def get_column_values(
except SupersetSecurityException as ex:
return self.response(403, message=ex.message)
- row_limit = apply_max_row_limit(app.config["FILTER_SELECT_ROW_LIMIT"])
+ conf = current_app.config
+ row_limit = apply_max_row_limit(conf["FILTER_SELECT_ROW_LIMIT"])
Review Comment:
### Unclear configuration variable abbreviation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'conf' variable name is an unclear abbreviation of 'configuration' and
is only used once, making it an unnecessary intermediate variable.
###### Why this matters
Using abbreviations reduces code clarity and creates cognitive overhead. The
intermediate variable adds no value since it's used only once.
###### Suggested change ∙ *Feature Preview*
```python
row_limit =
apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a12429b4-a5e5-4041-a849-acdca6d1d8fe/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a12429b4-a5e5-4041-a849-acdca6d1d8fe?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a12429b4-a5e5-4041-a849-acdca6d1d8fe?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a12429b4-a5e5-4041-a849-acdca6d1d8fe?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a12429b4-a5e5-4041-a849-acdca6d1d8fe)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:211fddb3-ddc2-4321-b188-e635567df909 -->
[](211fddb3-ddc2-4321-b188-e635567df909)
##########
superset/views/error_handling.py:
##########
@@ -158,9 +160,10 @@
@app.errorhandler(HTTPException)
def show_http_exception(ex: HTTPException) -> FlaskResponse:
logger.warning("HTTPException", exc_info=True)
+
if (
"text/html" in request.accept_mimetypes
- and not app.config["DEBUG"]
+ and not conf["DEBUG"]
and ex.code in {404, 500}
):
Review Comment:
### Hardcoded MIME Type and Status Codes <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic string 'text/html' and magic numbers {404, 500} are hardcoded without
explanation or constants.
###### Why this matters
Hardcoded values without context make the code less maintainable and harder
to understand the significance of these specific values.
###### Suggested change ∙ *Feature Preview*
```python
HTML_MIME_TYPE = "text/html"
ERROR_PAGES = {404, 500} # Client and Server Error pages
if (
HTML_MIME_TYPE in request.accept_mimetypes
and not config["DEBUG"]
and ex.code in ERROR_PAGES
):
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac27600e-ed8f-4b5b-a8b7-6c5610a558cc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac27600e-ed8f-4b5b-a8b7-6c5610a558cc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac27600e-ed8f-4b5b-a8b7-6c5610a558cc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac27600e-ed8f-4b5b-a8b7-6c5610a558cc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac27600e-ed8f-4b5b-a8b7-6c5610a558cc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:28be5652-9e4e-438c-b748-a55c7342cece -->
[](28be5652-9e4e-438c-b748-a55c7342cece)
##########
superset/commands/report/alert.py:
##########
@@ -162,15 +163,15 @@ def _execute_query(self) -> pd.DataFrame:
rendered_sql, ALERT_SQL_LIMIT
)
- if app.config["MUTATE_ALERT_QUERY"]:
+ if current_app.config["MUTATE_ALERT_QUERY"]:
limited_rendered_sql = (
self._report_schedule.database.mutate_sql_based_on_config(
limited_rendered_sql
)
)
executor, username = get_executor( # pylint:
disable=unused-variable
- executors=app.config["ALERT_REPORTS_EXECUTORS"],
+ executors=current_app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
Review Comment:
### Unclear Intent with Unused Variable <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The pylint disable comment for 'unused-variable' suggests unclear code
intention, as executor is assigned but not used.
###### Why this matters
The presence of unused variables with suppressed warnings makes the code's
purpose less clear and could hide potential bugs.
###### Suggested change ∙ *Feature Preview*
```python
_, username = get_executor(
executors=current_app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/051121b3-fbc9-4b02-9eca-e0ba306d219a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/051121b3-fbc9-4b02-9eca-e0ba306d219a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/051121b3-fbc9-4b02-9eca-e0ba306d219a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/051121b3-fbc9-4b02-9eca-e0ba306d219a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/051121b3-fbc9-4b02-9eca-e0ba306d219a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:92733e3e-a52f-49b7-8e87-53196f0156b2 -->
[](92733e3e-a52f-49b7-8e87-53196f0156b2)
##########
superset/databases/ssh_tunnel/models.py:
##########
@@ -32,7 +32,7 @@
ImportExportMixin,
)
-app_config = current_app.config
+conf = current_app.config
Review Comment:
### Unclear Configuration Variable Abbreviation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The variable name 'conf' is an unclear abbreviation of 'configuration' that
doesn't add value over the original 'config' name.
###### Why this matters
Using abbreviated names that don't provide additional context makes the code
less immediately understandable and breaks consistency with Flask's convention
of using 'config'.
###### Suggested change ∙ *Feature Preview*
```python
config = current_app.config
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a284c758-e745-4f20-af00-6210b6dc4d4c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a284c758-e745-4f20-af00-6210b6dc4d4c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a284c758-e745-4f20-af00-6210b6dc4d4c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a284c758-e745-4f20-af00-6210b6dc4d4c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a284c758-e745-4f20-af00-6210b6dc4d4c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ced5e598-c1d8-4cdb-ae4b-f60771584855 -->
[](ced5e598-c1d8-4cdb-ae4b-f60771584855)
##########
superset/db_engine_specs/__init__.py:
##########
@@ -166,7 +167,8 @@ def get_available_engine_specs() ->
dict[type[BaseEngineSpec], set[str]]: # noq
driver = driver.decode()
drivers[backend].add(driver)
- dbs_denylist = app.config["DBS_AVAILABLE_DENYLIST"]
+ conf = current_app.config
+ dbs_denylist = conf["DBS_AVAILABLE_DENYLIST"]
Review Comment:
### Redundant Config Variable Assignment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary intermediate variable assignment for config access.
###### Why this matters
Creating an intermediate variable 'conf' adds memory overhead without
providing any performance benefit, especially since it's only used once.
###### Suggested change ∙ *Feature Preview*
Access the config value directly:
```python
dbs_denylist = current_app.config["DBS_AVAILABLE_DENYLIST"]
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a21fbbf2-64d5-4a4d-8f76-ded37a262b24/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a21fbbf2-64d5-4a4d-8f76-ded37a262b24?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a21fbbf2-64d5-4a4d-8f76-ded37a262b24?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a21fbbf2-64d5-4a4d-8f76-ded37a262b24?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a21fbbf2-64d5-4a4d-8f76-ded37a262b24)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4b08efd6-b467-4800-8e05-17f57c49a9bc -->
[](4b08efd6-b467-4800-8e05-17f57c49a9bc)
##########
superset/cli/main.py:
##########
@@ -22,28 +22,46 @@
import click
from colorama import Fore, Style
+from flask import current_app
from flask.cli import FlaskGroup, with_appcontext
-from superset import app, appbuilder, cli, security_manager
-from superset.cli.lib import normalize_token
+from superset import appbuilder, cli, security_manager
from superset.extensions import db
from superset.utils.decorators import transaction
logger = logging.getLogger(__name__)
+def normalize_token(token_name: str) -> str:
+ """
+ As of click>=7, underscores in function names are replaced by dashes.
+ To avoid the need to rename all cli functions, e.g. load_examples to
+ load-examples, this function is used to convert dashes back to
+ underscores.
+
+ :param token_name: token name possibly containing dashes
+ :return: token name where dashes are replaced with underscores
+ """
+ return token_name.replace("_", "-")
+
+
+def create_app() -> Any:
+ """Create app instance for CLI"""
+ from superset.app import create_app as create_superset_app
+
+ return create_superset_app()
+
+
@click.group(
cls=FlaskGroup,
+ create_app=create_app,
context_settings={"token_normalize_func": normalize_token},
)
@with_appcontext
def superset() -> None:
"""\033[1;37mThe Apache Superset CLI\033[0m"""
# NOTE: codes above are ANSI color codes for bold white in CLI header ^^^
-
- @app.shell_context_processor
- def make_shell_context() -> dict[str, Any]:
- return {"app": app, "db": db}
+ pass
Review Comment:
### Hardcoded ANSI codes in docstring <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The docstring contains hardcoded ANSI color codes which make it harder to
read and understand the purpose of the function at a glance.
###### Why this matters
Embedding raw ANSI codes in docstrings reduces readability and makes
maintenance more difficult. These formatting concerns should be handled
separately from the documentation.
###### Suggested change ∙ *Feature Preview*
Move the ANSI formatting to a constant or utility function and keep the
docstring clean:
```python
CLI_HEADER = "\033[1;37mThe Apache Superset CLI\033[0m"
@click.group(...)
@with_appcontext
def superset() -> None:
"""The Apache Superset CLI"""
pass
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9e89fb98-1a54-4e05-87bc-cf7500e4d5cd/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9e89fb98-1a54-4e05-87bc-cf7500e4d5cd?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9e89fb98-1a54-4e05-87bc-cf7500e4d5cd?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9e89fb98-1a54-4e05-87bc-cf7500e4d5cd?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9e89fb98-1a54-4e05-87bc-cf7500e4d5cd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6d87f5a3-071d-40de-aa82-e2728e777e99 -->
[](6d87f5a3-071d-40de-aa82-e2728e777e99)
##########
superset/utils/database.py:
##########
@@ -64,10 +64,14 @@ def get_or_create_db(
def get_example_database() -> Database:
+ # pylint: disable=import-outside-toplevel
Review Comment:
### Unexplained repeated pylint suppressions <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The pylint directive comments appear redundantly in multiple functions
without explaining why imports are outside the toplevel.
###### Why this matters
Developers need to understand why this design choice was made. Without
explanation, it's unclear if this is an intentional design pattern or a
technical debt item.
###### Suggested change ∙ *Feature Preview*
Add a module-level comment explaining the reason for delayed imports:
```python
# Note: Database imports are delayed to avoid circular dependencies
# and to optimize application startup time.
```
Then move the pylint directive to the module level:
```python
# pylint: disable=import-outside-toplevel # delayed imports explained above
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9d732f5-173b-43e0-a12a-586598ac612a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9d732f5-173b-43e0-a12a-586598ac612a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9d732f5-173b-43e0-a12a-586598ac612a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9d732f5-173b-43e0-a12a-586598ac612a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9d732f5-173b-43e0-a12a-586598ac612a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9c1ecc83-5450-4bb5-baa3-9d9c06cd6052 -->
[](9c1ecc83-5450-4bb5-baa3-9d9c06cd6052)
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
