villebro commented on code in PR #19890:
URL: https://github.com/apache/superset/pull/19890#discussion_r874393201
##########
superset/db_engine_specs/base.py:
##########
@@ -930,7 +932,11 @@ def extract_errors(
]
@classmethod
- def adjust_database_uri(cls, uri: URL, selected_schema: Optional[str]) ->
None:
+ def adjust_database_uri( # pylint: disable=unused-argument
+ cls,
+ uri: URL,
+ selected_schema: Optional[str],
+ ) -> URL:
Review Comment:
Minor comment: We may want to consider renaming and/or updating the
docstrings on this and other methods
like `modify_url_for_impersonation` that no longer mutate the original
object. For instance, `modify_url_for_impersonation` could be renamed
`get_url_for_impersonation` to emphasize that it's no longer mutating the
original object.
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -114,13 +114,15 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec):
@classmethod
def adjust_database_uri(
cls, uri: URL, selected_schema: Optional[str] = None
- ) -> None:
+ ) -> URL:
database = uri.database
if "/" in uri.database:
database = uri.database.split("/")[0]
if selected_schema:
selected_schema = parse.quote(selected_schema, safe="")
- uri.database = database + "/" + selected_schema
+ uri = uri.set(database=database + "/" + selected_schema)
Review Comment:
Maybe we could refactor this to an f-string (looks cleaner IMO):
```suggestion
uri = uri.set(database=f"{database}/{selected_schema}")
```
##########
superset/db_engine_specs/presto.py:
##########
@@ -33,7 +33,7 @@
from sqlalchemy import Column, literal_column, types
from sqlalchemy.engine.base import Engine
from sqlalchemy.engine.reflection import Inspector
-from sqlalchemy.engine.result import RowProxy
+from sqlalchemy.engine.result import Row as RowProxy
Review Comment:
To disambiguate from the old `RowProxy` class, maybe we should rename this
to `ResultRow`?
--
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]
