mikebridge commented on PR #40129: URL: https://github.com/apache/superset/pull/40129#issuecomment-4604884071
Thanks @aminghadersohi for the thorough re-review. Both outstanding items addressed in 30e0a3996b: **BLOCKER — test inline imports (`restore_test.py`)**: all eight moved to module top. The imports (`RestoreChartCommand`, `ChartNotFoundError`, `ChartForbiddenError`, `SupersetSecurityException`) are pure Python class definitions with no app-context dependency and no circular dependency — there was no justification for the inline form. Also took the consistency win on `datetime(...)` calls (added `tzinfo=timezone.utc`). **NIT — integration test inline imports (`soft_delete_tests.py`)**: both moved to module top. `dashboard_slices` is in the same module as `Dashboard` (already at top); `ReportSchedule`/etc. are SQLAlchemy models that are safe to import at module top in a `SupersetTestCase` integration context. **MEDIUM — `down_revision` 7 revisions behind master**: updated `7c4a8d09ca37`'s `down_revision` from `33d7e0e21daa` to `a1b2c3d4e5f6` (the current alembic head). The sibling dashboards (#40128) and datasets (#40130) PRs got the same update on their migrations in companion commits. Cross-PR merge-order coordination still applies — whichever lands first stays at `a1b2c3d4e5f6`, the others rebase onto the newly-merged revision. The branch was also rebased on current master in this push (the previous base was 34 commits behind preset/master; no conflicts). Thanks again — and good catch on retracting the first BLOCKER (`find_existing_for_import` already at module top from an earlier round). -- 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]
