aminghadersohi commented on PR #42571: URL: https://github.com/apache/superset/pull/42571#issuecomment-5183790997
Thanks for the review! I don't think this is actually a blocker — I believe the two assertions you're pointing at reference a different (unrelated) pair of constants than the ones this PR changed. There are two similarly-named-but-distinct constants in play here: - `SORTABLE_DATASET_COLUMNS` / `SORTABLE_DASHBOARD_COLUMNS` — local module-level constants defined near the top of `list_datasets.py` / `list_dashboards.py`. These are **not** used at runtime (dead/unused leftovers) and are what `test_dataset_sortable_columns_definition` / `test_dashboard_sortable_columns_definition` assert against. This PR does not touch them, so their `"changed_on_delta_humanized" not in ...` assertions still hold. - `DATASET_SORTABLE_COLUMNS` / `DASHBOARD_SORTABLE_COLUMNS` — imported from `common/schema_discovery.py`, actually passed to `ModelListCore(sortable_columns=...)` in both tool functions, and the ones this PR updates to add `changed_on_delta_humanized`. Since the flagged tests check the former (unused) constants, they're unaffected by this change. I verified this two ways on head `dc2fbafe11469f4dbae870504059687a81c29f20`: - Local pytest run: `test_dataset_sortable_columns_definition PASSED`, `test_dashboard_sortable_columns_definition PASSED` (2 passed in 1.99s). - CI on that same head: `unit-tests (current)` and `unit-tests-required` both passed. So no test/code change is needed here. Separately, I agree those two dead local constants are confusing precisely because of this near-miss — I called that out as a follow-up cleanup opportunity for a future PR, but didn't want to fold an unrelated dead-code removal into this fix. Happy to split that out if useful. -- 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]
