[GitHub] [superset] john-bodley commented on a diff in pull request #24465: chore(dao): Add generic type

["john-bodley (via GitHub)" <gi...@apache.org>]

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


##########
superset/daos/base.py:
##########
@@ -141,30 +145,23 @@ def create(cls, properties: dict[str, Any], commit: bool = True) -> Model:
         return model
 
     @classmethod
-    def save(cls, instance_model: Model, commit: bool = True) -> Model:
+    def save(cls, instance_model: T, commit: bool = True) -> None:
         """
         Generic for saving models
         :raises: DAOCreateFailedError
         """
         if cls.model_cls is None:
             raise DAOConfigError()
-        if not isinstance(instance_model, cls.model_cls):
-            raise DAOCreateFailedError(
-                "the instance model is not a type of the model class"
-            )
         try:
             db.session.add(instance_model)
             if commit:
                 db.session.commit()
         except SQLAlchemyError as ex:  # pragma: no cover
             db.session.rollback()
             raise DAOCreateFailedError(exception=ex) from ex
-        return instance_model

Review Comment:
   Inconsistent return type detected with the `ChartDAO` implementation. Note there's no need to return `instance_model` as it's passed in as in input.



##########
superset/annotation_layers/annotations/commands/delete.py:
##########
@@ -38,6 +38,8 @@ def __init__(self, model_id: int):
 
     def run(self) -> Model:
         self.validate()
+        assert self._model

Review Comment:
   This was needed because now Mypy correctly identified that on line #44 `self._model` could have been `None` which didn't adhere to the base class definition.



##########
superset/daos/base.py:
##########
@@ -141,30 +145,23 @@ def create(cls, properties: dict[str, Any], commit: bool = True) -> Model:
         return model
 
     @classmethod
-    def save(cls, instance_model: Model, commit: bool = True) -> Model:
+    def save(cls, instance_model: T, commit: bool = True) -> None:
         """
         Generic for saving models
         :raises: DAOCreateFailedError
         """
         if cls.model_cls is None:
             raise DAOConfigError()
-        if not isinstance(instance_model, cls.model_cls):

Review Comment:
   This is no longer needed. The Mypy checks ensure this can't happen.



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