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