NoiceHax commented on PR #43150:
URL: https://github.com/apache/superset/pull/43150#issuecomment-5296850417
Pushed a follow-up for the review comments.
On the data-test vs data-testid flag from the bots: that one is a false
positive. spec/helpers/setup.ts calls configureTestingLibrary({
testIdAttribute: 'data-test' }), so queryByTestId('render-error') already
resolves the data-test node. I left the query as is and added a short comment
pointing at the setup file so it does not get flagged again.
Took the two Copilot points. Dropped the (orig as any) casts, orig is
already Record<string, unknown> so the id probes type check without them, and
reworded the comments that said sorting the keys detects column order changes.
Sorting does the opposite, it makes the key independent of column order.
One extra bit of evidence for the fix itself. Running oxlint on the file as
it was before this PR reports five react-hooks/rules-of-hooks 'called
conditionally' warnings, which are exactly the five hooks this PR moves above
the early return. After the change all five are gone and only the pre-existing
exhaustive-deps warning is left.
I still could not run jest locally (no node_modules in this environment), so
the new tests are only covered by CI, which is green.
--
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]
