[GitHub] [incubator-superset] villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs
villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs URL: https://github.com/apache/incubator-superset/pull/9138#discussion_r379924285 ## File path: superset/db_engine_specs/snowflake.py ## @@ -47,14 +49,13 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec): } @classmethod -def adjust_database_uri(cls, uri, selected_schema=None): +def adjust_database_uri(cls, uri: URL, selected_schema: str = None) -> None: Review comment: This is still missing an `Optional` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs
villebro commented on a change in pull request #9138: [mypy] Enforcing typing
for db_engine_specs
URL:
https://github.com/apache/incubator-superset/pull/9138#discussion_r379492319
##
File path: superset/db_engine_specs/base.py
##
@@ -486,14 +496,13 @@ def extract_error_message(cls, e: Exception) -> str:
return f"{cls.engine} error: {cls._extract_error_message(e)}"
@classmethod
-def _extract_error_message(cls, e: Exception) -> str:
+def _extract_error_message(cls, e: Exception) -> Optional[str]:
"""Extract error message for queries"""
return utils.error_msg_from_exception(e)
@classmethod
-def adjust_database_uri(cls, uri, selected_schema: Optional[str]):
-"""Based on a URI and selected schema, return a new URI
-
+def adjust_database_uri(cls, uri: URL, selected_schema: Optional[str]) ->
None:
Review comment:
If memory serves me right, these wouldn't benefit from chaining in the
context they're used. However, I don't oppose returning the mutated object, but
in that case I'd add a note in the docstring. Also there's a `# TODO:` below
that can be removed.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs
villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs URL: https://github.com/apache/incubator-superset/pull/9138#discussion_r379489998 ## File path: superset/db_engine_specs/base.py ## @@ -472,7 +482,7 @@ def get_all_datasource_names( return all_datasources @classmethod -def handle_cursor(cls, cursor, query, session): +def handle_cursor(cls, cursor: Any, query: Select, session: Session) -> None: Review comment: I think here `query` is actually a `from superset.models.sql_lab import Query` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs
villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs URL: https://github.com/apache/incubator-superset/pull/9138#discussion_r379494767 ## File path: superset/db_engine_specs/presto.py ## @@ -712,7 +720,7 @@ def get_create_view(cls, database, schema: str, table: str) -> Optional[str]: return rows[0][0] @classmethod -def handle_cursor(cls, cursor, query, session): +def handle_cursor(cls, cursor: Any, query: Select, session: Session) -> None: Review comment: same as above 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs
villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs URL: https://github.com/apache/incubator-superset/pull/9138#discussion_r379495119 ## File path: superset/db_engine_specs/snowflake.py ## @@ -47,14 +49,13 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec): } @classmethod -def adjust_database_uri(cls, uri, selected_schema=None): +def adjust_database_uri(cls, uri: URL, selected_schema: str = None) -> None: Review comment: Missing `Optional[]` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs
villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs URL: https://github.com/apache/incubator-superset/pull/9138#discussion_r379493972 ## File path: superset/db_engine_specs/hive.py ## @@ -192,24 +195,23 @@ def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]: return None @classmethod -def adjust_database_uri(cls, uri, selected_schema=None): +def adjust_database_uri(cls, uri: URL, selected_schema: str = None) -> None: Review comment: I think `schema` type is missing `Optional[]` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs
villebro commented on a change in pull request #9138: [mypy] Enforcing typing for db_engine_specs URL: https://github.com/apache/incubator-superset/pull/9138#discussion_r379494124 ## File path: superset/db_engine_specs/hive.py ## @@ -237,15 +239,17 @@ def progress(cls, log_lines): return int(progress) @classmethod -def get_tracking_url(cls, log_lines): +def get_tracking_url(cls, log_lines: List[str]) -> Optional[str]: lkp = "Tracking URL = " for line in log_lines: if lkp in line: return line.split(lkp)[1] -return None +return None @classmethod -def handle_cursor(cls, cursor, query, session): # pylint: disable=too-many-locals +def handle_cursor( # pylint: disable=too-many-locals +cls, cursor: Any, query: Select, session: Session Review comment: Same as above 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
