You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/06/20 21:26:51 UTC

[GitHub] [superset] john-bodley opened a new pull request, #24466: chore(dao): Condense delete/bulk-delete

john-bodley opened a new pull request, #24466:
URL: https://github.com/apache/superset/pull/24466

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   
   Per  [SIP-92 Proposal for restructuring the Python code base](https://github.com/apache/superset/issues/20630) https://github.com/apache/superset/pull/24331 organized all the DAOs to be housed within a shared folder—the result of which highlighted numerous inconsistencies, repetition, and inefficiencies.
   
   This PR (one of many smaller bitesized PRs) condenses the single and bulk deletion logic into a single method which removes a bunch of copypasta and helps to standardize the code.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: https://github.com/apache/superset/issues/20630
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


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


[GitHub] [superset] john-bodley commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1236313105


##########
superset/annotation_layers/annotations/commands/delete.py:
##########
@@ -36,14 +34,15 @@ def __init__(self, model_id: int):
         self._model_id = model_id
         self._model: Optional[Annotation] = None
 
-    def run(self) -> Model:
+    def run(self) -> None:
         self.validate()
+        assert self._model
+
         try:
-            annotation = AnnotationDAO.delete(self._model)

Review Comment:
   No need to return the object we're deleting.



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


[GitHub] [superset] john-bodley merged pull request #24466: chore(dao): Condense delete/bulk-delete operations

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #24466:
URL: https://github.com/apache/superset/pull/24466


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


[GitHub] [superset] john-bodley commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete operations

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1235912205


##########
superset/daos/annotation.py:
##########
@@ -67,21 +51,6 @@ def validate_update_uniqueness(
 class AnnotationLayerDAO(BaseDAO):
     model_cls = AnnotationLayer
 
-    @staticmethod

Review Comment:
   Dito.



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


[GitHub] [superset] codecov[bot] commented on pull request #24466: chore(dao): Condense delete/bulk-delete

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24466:
URL: https://github.com/apache/superset/pull/24466#issuecomment-1599597448

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#24466](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f7582fc) into [master](https://app.codecov.io/gh/apache/superset/commit/c3b5d72f2b078eff4ff21213085f0d4ad6d282e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c3b5d72) will **decrease** coverage by `10.67%`.
   > The diff coverage is `65.41%`.
   
   > :exclamation: Current head f7582fc differs from pull request most recent head 0d219ef. Consider uploading reports for the commit 0d219ef to get more accurate results
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #24466       +/-   ##
   ===========================================
   - Coverage   68.85%   58.18%   -10.67%     
   ===========================================
     Files        1901     1901               
     Lines       73969    73956       -13     
     Branches     8119     8116        -3     
   ===========================================
   - Hits        50931    43031     -7900     
   - Misses      20927    28814     +7887     
     Partials     2111     2111               
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `53.96% <60.80%> (?)` | |
   | postgres | `?` | |
   | presto | `53.86% <57.60%> (?)` | |
   | python | `60.92% <68.00%> (-22.21%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `54.66% <61.60%> (+0.06%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...ckages/superset-ui-chart-controls/src/constants.ts](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2NvbnN0YW50cy50cw==) | `100.00% <ø> (ø)` | |
   | [...erset-ui-chart-controls/src/utils/columnChoices.ts](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3V0aWxzL2NvbHVtbkNob2ljZXMudHM=) | `100.00% <ø> (ø)` | |
   | [...packages/superset-ui-core/src/query/types/Query.ts](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/SqlLab/actions/sqlLab.js](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `69.16% <ø> (-0.27%)` | :arrow_down: |
   | [...d/src/SqlLab/components/SaveDatasetModal/index.tsx](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwvaW5kZXgudHN4) | `50.00% <0.00%> (ø)` | |
   | [superset-frontend/src/SqlLab/fixtures.ts](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
   | [...rset-frontend/src/explore/components/SaveModal.tsx](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwudHN4) | `35.08% <ø> (+0.30%)` | :arrow_up: |
   | [...otation\_layers/annotations/commands/bulk\_delete.py](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvYW5ub3RhdGlvbl9sYXllcnMvYW5ub3RhdGlvbnMvY29tbWFuZHMvYnVsa19kZWxldGUucHk=) | `50.00% <0.00%> (-37.50%)` | :arrow_down: |
   | [superset/annotation\_layers/commands/bulk\_delete.py](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvYW5ub3RhdGlvbl9sYXllcnMvY29tbWFuZHMvYnVsa19kZWxldGUucHk=) | `46.15% <0.00%> (-38.47%)` | :arrow_down: |
   | [superset/charts/commands/bulk\_delete.py](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2J1bGtfZGVsZXRlLnB5) | `44.44% <0.00%> (-46.99%)` | :arrow_down: |
   | ... and [37 more](https://app.codecov.io/gh/apache/superset/pull/24466?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
   
   ... and [265 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24466/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


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


[GitHub] [superset] john-bodley commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1237281247


##########
superset/dashboards/api.py:
##########
@@ -1364,7 +1364,7 @@ def delete_embedded(self, dashboard: Dashboard) -> Response:
               $ref: '#/components/responses/500'
         """
         for embedded in dashboard.embedded:
-            DashboardDAO.delete(embedded)
+            EmbeddedDashboardDAO.delete(embedded)

Review Comment:
   I'm somewhat surprised that https://github.com/apache/superset/pull/24465 didn't pick this up.



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


[GitHub] [superset] john-bodley commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1235912485


##########
superset/daos/annotation.py:
##########
@@ -67,21 +51,6 @@ def validate_update_uniqueness(
 class AnnotationLayerDAO(BaseDAO):
     model_cls = AnnotationLayer
 
-    @staticmethod
-    def bulk_delete(

Review Comment:
   Copypasta except the `BaseDAO` is defined as a `classmethod` as opposed to a `staticmethod`.



##########
superset/daos/css.py:
##########
@@ -15,31 +15,12 @@
 # specific language governing permissions and limitations
 # under the License.
 import logging
-from typing import Optional
-
-from sqlalchemy.exc import SQLAlchemyError
 
 from superset.daos.base import BaseDAO
-from superset.daos.exceptions import DAODeleteFailedError
-from superset.extensions import db
 from superset.models.core import CssTemplate
 
 logger = logging.getLogger(__name__)
 
 
 class CssTemplateDAO(BaseDAO):
     model_cls = CssTemplate
-
-    @staticmethod

Review Comment:
   Copypasta except the `BaseDAO` is defined as a `classmethod` as opposed to a `staticmethod`.



##########
superset/daos/annotation.py:
##########
@@ -30,19 +27,6 @@
 class AnnotationDAO(BaseDAO):
     model_cls = Annotation
 
-    @staticmethod

Review Comment:
   Copypasta except the `BaseDAO` is defined as a `classmethod` as opposed to a `staticmethod`.



##########
superset/daos/annotation.py:
##########
@@ -67,21 +51,6 @@ def validate_update_uniqueness(
 class AnnotationLayerDAO(BaseDAO):
     model_cls = AnnotationLayer
 
-    @staticmethod

Review Comment:
   Same logic as the `BaseDAO`, though was defined as a static-method.



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


[GitHub] [superset] john-bodley commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1237349975


##########
superset/annotation_layers/annotations/commands/bulk_delete.py:
##########
@@ -36,8 +36,10 @@ def __init__(self, model_ids: list[int]):
 
     def run(self) -> None:
         self.validate()
+        assert self._models

Review Comment:
   This logic is temporary. It _should_ be cleaned up when we refactor the commands and enforce that `validate` doesn't augment any properties.



##########
superset/daos/dataset.py:
##########
@@ -318,28 +318,26 @@ def create_column(
         return DatasetColumnDAO.create(properties, commit=commit)
 
     @classmethod
-    def delete_column(cls, model: TableColumn, commit: bool = True) -> TableColumn:
+    def delete_column(cls, model: TableColumn, commit: bool = True) -> None:
         """
         Deletes a Dataset column
         """
-        return cls.delete(model, commit=commit)
+        return DatasetColumnDAO.delete(model, commit=commit)

Review Comment:
   This is more accurate, i.e., the `DatasetColumnDAO` rather than the `DatasetDAO` should be deleting the column.



##########
superset/daos/base.py:
##########
@@ -179,26 +183,33 @@ def update(cls, model: T, properties: dict[str, Any], commit: bool = True) -> T:
             raise DAOUpdateFailedError(exception=ex) from ex
         return model
 
+    @overload
+    @classmethod
+    def delete(cls, objects: list[T], commit: bool = True) -> None:
+        ...
+
+    @overload
+    @classmethod
+    def delete(cls, objects: T, commit: bool = True) -> None:
+        ...
+
     @classmethod
-    def delete(cls, model: T, commit: bool = True) -> T:
+    def delete(cls, objects: T | list[T], commit: bool = True) -> None:

Review Comment:
   `objects` seems to be a more accurate than `models`, i.e., we're deleting database objects as opposed to models.



##########
superset/datasets/metrics/commands/delete.py:
##########
@@ -40,13 +38,13 @@ def __init__(self, dataset_id: int, model_id: int):
         self._model_id = model_id
         self._model: Optional[SqlMetric] = None
 
-    def run(self) -> Model:
+    def run(self) -> None:
         self.validate()
+        assert self._model
+
         try:
-            if not self._model:

Review Comment:
   DRY. The `validate` method already raises a `DatasetMetricNotFoundError` if the metric is not found.



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


[GitHub] [superset] john-bodley commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1237973991


##########
superset/daos/dataset.py:
##########
@@ -361,24 +359,27 @@ def create_metric(
         """
         return DatasetMetricDAO.create(properties, commit=commit)
 
-    @staticmethod
-    def bulk_delete(models: Optional[list[SqlaTable]], commit: bool = True) -> None:
-        item_ids = [model.id for model in models] if models else []
+    @classmethod
+    def delete(

Review Comment:
   The `bulk_deletion` logic differs from the `delete` logic in the sense that the former first deletes the relationships (which in actuality may not be explicitly required) before deleting the dataset, whereas the later only deletes the datasets (which in theory _should_ delete all the records with an explicit relationship).



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


[GitHub] [superset] michael-s-molina commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete operations

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1254366942


##########
superset/daos/base.py:
##########
@@ -180,25 +183,28 @@ def update(cls, model: T, properties: dict[str, Any], commit: bool = True) -> T:
         return model
 
     @classmethod
-    def delete(cls, model: T, commit: bool = True) -> T:
+    def delete(cls, items: T | list[T], commit: bool = True) -> None:
         """
-        Generic delete a model
-        :raises: DAODeleteFailedError
+        Delete the specified items(s) including their associated relationships.

Review Comment:
   ```suggestion
           Delete the specified item(s) including their associated relationships.
   ```



##########
superset/daos/annotation.py:
##########
@@ -67,21 +51,6 @@ def validate_update_uniqueness(
 class AnnotationLayerDAO(BaseDAO):
     model_cls = AnnotationLayer
 
-    @staticmethod
-    def bulk_delete(

Review Comment:
   Nice cleanup!



##########
superset/daos/annotation.py:
##########
@@ -30,19 +27,6 @@
 class AnnotationDAO(BaseDAO):
     model_cls = Annotation
 
-    @staticmethod

Review Comment:
   These comments are very helpful while reviewing. Thank you.



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


[GitHub] [superset] michael-s-molina commented on pull request #24466: chore(dao): Condense delete/bulk-delete operations

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24466:
URL: https://github.com/apache/superset/pull/24466#issuecomment-1623627432

   Thank you for the nice cleanup @john-bodley!


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


[GitHub] [superset] john-bodley commented on a diff in pull request #24466: chore(dao): Condense delete/bulk-delete operations

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24466:
URL: https://github.com/apache/superset/pull/24466#discussion_r1237354231


##########
superset/daos/base.py:
##########
@@ -179,26 +183,33 @@ def update(cls, model: T, properties: dict[str, Any], commit: bool = True) -> T:
             raise DAOUpdateFailedError(exception=ex) from ex
         return model
 
+    @overload
+    @classmethod
+    def delete(cls, objects: list[T], commit: bool = True) -> None:
+        ...
+
+    @overload
+    @classmethod
+    def delete(cls, objects: T, commit: bool = True) -> None:
+        ...
+
     @classmethod
-    def delete(cls, model: T, commit: bool = True) -> T:
+    def delete(cls, objects: T | list[T], commit: bool = True) -> None:

Review Comment:
   `items` seems to be a more accurate/consistent than `models`, i.e., we're deleting database items/instances as opposed to models.



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