john-bodley commented on code in PR #24700:
URL: https://github.com/apache/superset/pull/24700#discussion_r1264183672


##########
superset/commands/exceptions.py:
##########
@@ -44,7 +44,7 @@ def __init__(
         super().__init__(
             _(
                 self.message_format.format(
-                    object_type, '"%s" ' % object_id if object_id else ""
+                    object_type, f'"{object_id}" ' if object_id else ""

Review Comment:
   Just wondering whether the trailing space should be there. I realize it was 
previously, but I'm not sure if that was a typo.



##########
superset/db_engine_specs/ocient.py:
##########
@@ -238,7 +238,7 @@ class OcientEngineSpec(BaseEngineSpec):
     # Store mapping of superset Query id -> Ocient ID
     # These are inserted into the cache when executing the query
     # They are then removed, either upon cancellation or query completion
-    query_id_mapping: dict[str, str] = dict()

Review Comment:
   Love it.



##########
superset/db_engine_specs/presto.py:
##########
@@ -619,12 +619,13 @@ def latest_sub_partition(
                 raise SupersetTemplateException(msg)
         if len(kwargs.keys()) != len(part_fields) - 1:
             msg = (
-                "A filter needs to be specified for {} out of the " "{} 
fields."
-            ).format(len(part_fields) - 1, len(part_fields))
+                f"A filter needs to be specified for {len(part_fields) - 1} 
out of the "
+                f"{len(part_fields)} fields."
+            )
             raise SupersetTemplateException(msg)
 
         for field in part_fields:
-            if field not in kwargs.keys():

Review Comment:
   OooO.



##########
superset/charts/data/api.py:
##########
@@ -143,7 +143,7 @@ def get_data(self, pk: int) -> Response:
             query_context = self._create_query_context_from_form(json_body)
             command = ChartDataCommand(query_context)
             command.validate()
-        except DatasourceNotFound as error:

Review Comment:
   I know some people find Pylint to be somewhat cumbersome, but I love to see 
Pylint every evolving which results in fixes like this—which typically `flake8` 
et al. miss.



##########
superset/charts/data/api.py:
##########
@@ -143,7 +143,7 @@ def get_data(self, pk: int) -> Response:
             query_context = self._create_query_context_from_form(json_body)
             command = ChartDataCommand(query_context)
             command.validate()
-        except DatasourceNotFound as error:

Review Comment:
   I know some people find Pylint to be somewhat cumbersome, but I love to see 
Pylint every evolving which results in fixes like this—which typically `flake8` 
et al. miss.



-- 
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]

Reply via email to