Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cluster-ui: delete unused vars #107798

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Jul 28, 2023

Delete unused vars in cluster-ui. A following commit will turn unused vars to errors via the eslint config.

Epic: None

Release note: None

@xinhaoz xinhaoz requested review from a team July 28, 2023 15:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice cleanup!

Reviewed 32 of 33 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/columnsSelector/columnsSelector.tsx line 85 at r1 (raw file):

    boxShadow: "none",
  }),
  menuList: (provided: any) => ({

this is required, is setting the values on the customStyles, which is called on the code below

@xinhaoz
Copy link
Member Author

xinhaoz commented Jul 28, 2023

pkg/ui/workspaces/cluster-ui/src/columnsSelector/columnsSelector.tsx line 85 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this is required, is setting the values on the customStyles, which is called on the code below

Ah, gotcha + nice catch! This shouldn't affect the eslint rule anyways since it's a property. I'll put it back 👍

@xinhaoz
Copy link
Member Author

xinhaoz commented Jul 28, 2023

pkg/ui/workspaces/cluster-ui/src/columnsSelector/columnsSelector.tsx line 85 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Ah, gotcha + nice catch! This shouldn't affect the eslint rule anyways since it's a property. I'll put it back 👍

Done.

@xinhaoz xinhaoz requested a review from maryliag July 28, 2023 15:52
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts line 96 at r2 (raw file):

    }
  },
  handleMaxSizeError: () => {

I'm surprise this is not giving errors after you removed the values. On the DatabaseDetailsQuery type this function is defined as receiving these 3 parameters, but you removed from almost all implementations. Can you confirm this is really handling correctly the max size error?

@xinhaoz
Copy link
Member Author

xinhaoz commented Jul 28, 2023

pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts line 96 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I'm surprise this is not giving errors after you removed the values. On the DatabaseDetailsQuery type this function is defined as receiving these 3 parameters, but you removed from almost all implementations. Can you confirm this is really handling correctly the max size error?

I think it's because in JS/TS when a function is called with more arguments than there are params, theyre just ignored. One query that has handleMaxSizeError functions does use those params which is why theyre still included there. For consistency, instead of creating a union type to include functions without params, I've just appended the unused params with _ so we can continue to use the single type without confusion.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts line 96 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I think it's because in JS/TS when a function is called with more arguments than there are params, theyre just ignored. One query that has handleMaxSizeError functions does use those params which is why theyre still included there. For consistency, instead of creating a union type to include functions without params, I've just appended the unused params with _ so we can continue to use the single type without confusion.

Thanks!

Delete unused vars in cluster-ui. A following commit will
turn unused vars to errors via the eslint config.

Release note: None

Epic: none
@xinhaoz
Copy link
Member Author

xinhaoz commented Jul 28, 2023

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Build succeeded:

@craig craig bot merged commit 7ba8059 into cockroachdb:master Jul 28, 2023
@xinhaoz xinhaoz deleted the delete-unused branch April 1, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants