rusackas commented on PR #41549:
URL: https://github.com/apache/superset/pull/41549#issuecomment-5106818458
Just noting the bito comments tucked away under the accordion menu. Not sure
if you want to nab them now, or in a follow-up:
- superset/cli/deletion_retention.py - 1
- Missing CLI-level integration tests · [Line
73-109](https://github.com/apache/superset/pull/41549/files#diff-f81ba0108b8d5a1b25f0ac50a93026947a19cd2b238a9babc49c2d59d5c49654R73)
- The new CLI commands (set_window, show_window, force_purge) have no
CLI-level integration tests. Tests exist for the underlying ForcePurgeCommand
and resolve_retention_window() but not for the click entry points. Without CLI
tests, regressions in argument parsing, confirmation prompts, or output
formatting will not be caught.
- docker/pythonpath_dev/superset_config.py - 1
- Missing feature flag for dev · [Line
87-87](https://github.com/apache/superset/pull/41549/files#diff-415de08cbf9e8633d6cf2c439df7aa08bf9ebce684833ee837d6ce7d7ab7d53bR87)
- The import is correctly configured, but the scheduled task will silently
skip because `SOFT_DELETE` defaults to `False` (config.py:698). Developers
deploying this config may not realize the feature gate is off. Consider
documenting the required flag or enabling it for dev environments.
- tests/integration_tests/deletion_retention/window_tests.py - 2
- Incorrect type annotation dict[str, object] · [Line
78-78](https://github.com/apache/superset/pull/41549/files#diff-bb0cab67cb7fee1a284a1acdcd3fda7b6f478fd88286f4a781f377fa5ecbeb6cR78)
- The local variable `result` on line 78 is annotated `dict[str, object]`,
but `_purge_impl` (line 261, superset/tasks/deletion_retention.py) and
`purge_soft_deleted` (line 261) are both declared to return `dict[str, Any]`.
`Any` — not `object` — is the correct type parameter here. This causes a
type-checker to flag a type mismatch on every assertion that reads from
`result` (e.g., `result["purged"]`). Replace `object` with `Any` and add the
necessary import from `typing` if not already present.
- Replace broad Exception catch · [Line
51-51](https://github.com/apache/superset/pull/41549/files#diff-bb0cab67cb7fee1a284a1acdcd3fda7b6f478fd88286f4a781f377fa5ecbeb6cR51)
- The `except Exception` clause catches all exceptions without specific
handling. Consider catching a more specific exception type (e.g.,
`KeyValueDAO.NoEntryFoundError` or a generic `Exception` alongside targeted
logging).
- tests/integration_tests/deletion_retention/force_purge_tests.py - 1
- Test name mismatch with assertions · [Line
97-97](https://github.com/apache/superset/pull/41549/files#diff-fd823d9402f14d560469dac449363c6c1a2c1fde134ded4fa6468a4902882b73R97)
- The test name mentions 'reports_relationship_count' but the test body
verifies `removed_dashboard_slices` (a dashboard-chart relationship), not any
ReportSchedule field. A reader could incorrectly assume the test covers
report-blocking behavior. Rename to match the actual assertion.
--
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]
