rusackas commented on PR #36339:
URL: https://github.com/apache/superset/pull/36339#issuecomment-4605856426
Thanks for tackling this — deterministic keys are a real quality-of-life
improvement for anyone diffing dashboard exports. A few things to clean up
before this is ready:
- Remove `suffix()`— the `# No longer needed` comment is correct, so go
ahead and delete the function entirely rather than leaving dead code behind.
- Unsafe double-iteration over a set — `charts` is a `set[Slice]`, and you
iterate over it twice: once to build `chart_hashes`, then again in the `zip`.
Set iteration order isn't guaranteed to be consistent between the two passes,
which could mismatch a hash with the wrong chart. Converting to a list first
would fix this: `charts_list = list(charts)`.
- Row key collision on repeated calls —
`ROW-N-{len(position['GRID_ID']['children'])}` is computed before the new row
is appended, so if `append_charts` is called more than once on the same
position dict, you could end up with duplicate row keys. Worth adding a test
for that case (and a guard if needed).
- Test coverage gaps — the new test covers the happy path well, but consider
adding cases for: repeated calls to `append_charts`, and the branch where
`ROOT_ID/GRID_ID` are absent from the position.
None of these are blockers for correctness in the common case, but items 2
and 3 in particular are subtle bugs worth addressing. Great start overall!
--
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]
