Re: [PR] fix(mcp): accept changed_on_delta_humanized as order_column in list tools [superset]

["aminghadersohi (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org