mikebridge commented on PR #40130: URL: https://github.com/apache/superset/pull/40130#issuecomment-4606699147
Thanks @bito-code-review. Mixed response on the four items: **Addressed (13b9009fcb):** - ✅ One-line docstrings added to `upgrade()` and `downgrade()` in the migration. **Declining the filters.py:58-62 type annotation** (`Missing type annotation on model`): The parent class `BaseDeletedStateFilter` (in `superset/views/filters.py`) already declares the abstract attribute: ```python model: ClassVar[type[SoftDeleteMixin]] ``` A subclass assignment `model = SqlaTable` is type-checked against the parent's annotation by mypy. Declaring `model: ClassVar[type[SoftDeleteMixin]] = SqlaTable` on the subclass would be redundant. The sibling classes `ChartDeletedStateFilter` and `DashboardDeletedStateFilter` follow the same pattern (bare assignment), and mypy passes on all three. Adding the annotation here would diverge from the established pattern without a correctness benefit. **Test-coverage suggestions — gated by integration vs unit:** - *Missing test for conflict guard in `importers/v1/utils.py:239-249`*: the conflict guard is covered end-to-end by `test_restore_blocked_by_active_logical_duplicate` and `test_create_blocked_by_soft_deleted_logical_duplicate` in `tests/integration_tests/datasets/soft_delete_tests.py` (lines 200–289). Those tests seed an active dataset with a matching `(database, catalog, schema, table_name)`, exercise the restore + create paths, and assert 422 + the post-state. A unit test would add isolation but no new behavioural coverage. Defer to integration coverage here. - *Missing `DatasetRestoreFailedError` test path*: similarly, the integration suite covers the restore endpoint's failure mode end-to-end. A unit-level analogue to `test_restore_dataset_forbidden_raises` that patches the DAO to raise an unexpected exception would be defensible but doesn't pin any contract that the integration tests don't already cover. Both items are reasonable as additions in a follow-up PR if the maintainers prefer unit-level coverage of every command-level error path, but I don't see them as blockers for this PR. -- 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]
