You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2025/07/28 04:20:17 UTC
[PR] chore: proper current_app.config proxy usage [superset]
mistercrunch opened a new pull request, #34345:
URL: https://github.com/apache/superset/pull/34345
## Summary
This PR improves config access patterns throughout the Superset codebase to reduce circular import issues and prepare for better app factory patterns.
### Changes
1. **Migrated easy cases to use `current_app.config`**: Files with Flask routes/views that have guaranteed app context now use `current_app.config` directly instead of importing from `superset.__init__.py`
2. **Added `conf = current_app.config` alias**: For cleaner code, we use a short alias `conf` that references `current_app.config` (which is already a proxy)
3. **Removed module-scoped config extractions**: Moved config access from module scope into methods/functions where app context is available
4. **Added TODO markers**: Complex module-scoped cases are marked with `# TODO-CONF` for future refactoring
### Why These Changes?
The current pattern of using `from superset import app` and then `app.config` at module scope has several issues:
1. **Circular imports**: Since `superset/__init__.py` imports from many modules that then import back from it
2. **App factory pattern**: Module-scoped config access prevents proper app factory usage for testing with different configs
3. **Context requirements**: Flask's `current_app` is the recommended way to access config within request context
### Migration Strategy
- **Easy wins first**: Views, APIs, and routes that already have app context
- **Local variables**: For module-scoped variables, we create local variables within methods
- **Preserve behavior**: All changes maintain exact same functionality while improving the pattern
### Files Modified
- `superset/sqllab/api.py` - API endpoints
- `superset/available_domains/api.py` - API endpoints
- `superset/views/base.py` - View functions
- `superset/models/core.py` - Database model with complex config usage
- `superset/models/helpers.py` - Added local variable for ADVANCED_DATA_TYPES
- `superset/connectors/sqla/models.py` - Removed unused ADVANCED_DATA_TYPES
### Testing
- All changes are refactoring only - no functional changes
- Existing tests continue to pass
- Code compiles without import errors
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2245758681
##########
superset/advanced_data_type/api.py:
##########
@@ -29,8 +29,8 @@
from superset.extensions import event_logger
from superset.views.base_api import BaseSupersetApi
-config = app.config
-ADVANCED_DATA_TYPES = config["ADVANCED_DATA_TYPES"]
+conf = app.config
Review Comment:
yeah was going for concise referencing, but I agree it's better to not reference the proxy in module scope
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #34345:
URL: https://github.com/apache/superset/pull/34345
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2245758681
##########
superset/advanced_data_type/api.py:
##########
@@ -29,8 +29,8 @@
from superset.extensions import event_logger
from superset.views.base_api import BaseSupersetApi
-config = app.config
-ADVANCED_DATA_TYPES = config["ADVANCED_DATA_TYPES"]
+conf = app.config
Review Comment:
yeah was going for concise referencing, but I agree it's better to not reference the proxy in module scope ... saving 13 character out of our 88 per-line max...
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2243979520
##########
scripts/python_tests.sh:
##########
@@ -33,4 +33,4 @@ superset load-test-users
echo "Running tests"
-pytest --durations-min=2 --maxfail=1 --cov-report= --cov=superset ./tests/integration_tests "$@"
Review Comment:
If we stand up CI, might as well get a full report!
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2243990920
##########
superset/charts/schemas.py:
##########
@@ -624,13 +636,9 @@ class ChartDataProphetOptionsSchema(ChartDataPostProcessingOperationOptionsSchem
"[ISO 8601](https://en.wikipedia.org/wiki/ISO_8601#Durations) durations.",
"example": "P1D",
},
- validate=validate.OneOf(
- choices=[
- i
- for i in {**builtin_time_grains, **config["TIME_GRAIN_ADDONS"]}.keys()
- if i
- ]
- ),
+ # TODO: This validation should be dynamic based on config
+ # For now, we skip validation at field level and can add
+ # a @validates_schema method if needed
Review Comment:
ooops, gotta fix that
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2244914521
##########
superset/advanced_data_type/api.py:
##########
@@ -29,8 +29,8 @@
from superset.extensions import event_logger
from superset.views.base_api import BaseSupersetApi
-config = app.config
-ADVANCED_DATA_TYPES = config["ADVANCED_DATA_TYPES"]
+conf = app.config
Review Comment:
I'm not a fan of these module level references, what do you think about removing them and replacing `ADVANCED_DATA_TYPES` with `current_app.config["ADVANCED_DATA_TYPES"]` where needed?
##########
superset/commands/dashboard/filter_state/get.py:
##########
@@ -16,20 +16,23 @@
# under the License.
from typing import Optional
-from flask import current_app as app
+from flask import current_app
from superset.commands.dashboard.filter_state.utils import check_access
from superset.commands.temporary_cache.get import GetTemporaryCacheCommand
from superset.commands.temporary_cache.parameters import CommandParameters
from superset.extensions import cache_manager
from superset.temporary_cache.utils import cache_key
+conf = current_app.config
Review Comment:
can we avoid these module level references, they need an app context, so any imports to this module will need an app context also
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #34345:
URL: https://github.com/apache/superset/pull/34345#issuecomment-3125368422
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](20f843f1-b9fc-4abf-b08a-565684bc04e1)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2243999821
##########
superset/utils/cache.py:
##########
@@ -169,6 +169,11 @@ def etag_cache( # noqa: C901
def decorator(f: Callable[..., Any]) -> Callable[..., Any]: # noqa: C901
@wraps(f)
def wrapper(*args: Any, **kwargs: Any) -> Response: # noqa: C901
+ # Set default max_age if not provided
+ nonlocal max_age
Review Comment:
Wait, do we need `nonlocal` here?
##########
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:
You don't need `pass` if the function has a docstring.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34345:
URL: https://github.com/apache/superset/pull/34345#discussion_r2246168352
##########
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:
Good catch! You're right - the `pass` statement is redundant here since the function already has a docstring. Would you like me to update the suggested change to remove it?
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: proper current_app.config proxy usage [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #34345:
URL: https://github.com/apache/superset/pull/34345#issuecomment-3141093379
Implementing some changes based on @dpgaspar 's excellent feedback
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org