rusackas commented on PR #36315: URL: https://github.com/apache/superset/pull/36315#issuecomment-3590291208
A few suggestions/questions (thanks, Claude!) which you can take on or let me know if it's crazy talk: The `viz.py` change looks incomplete — they add `diff_delta` and `diff_secs calculations`, but `diff_secs` is never used. The original code presumably calculated these elsewhere, so this might break something. I'd want to see where `diff_delta` and `diff_secs` were originally defined to make sure this doesn't introduce a regression. Test coverage is thin — the test only checks one case (numeric range). It should also test the Date range branch: `const range = new Date(Date.UTC(2025, 1, 1)); // Feb 1st` Edge case for `range_ = diff_delta.years * 12 + diff_delta.months + 1` — after subtracting a day from end, if the original end was exactly the first of a month, the +1 might now be off-by-one. Would want a quick sanity check on boundary cases. Variable rename inconsistency — `stop` → `stopExclusive` is helpful for clarity, but the test comment says "2025-02-31" which isn't a valid date. Claude's recommendation: Request minor changes — ask the contributor to verify the `viz.py` logic (especially unused `diff_secs`), add one more test case for the Date range path, and fix the test comment. Then it's good to merge. -- 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]
