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

refactor: TypeScript Type Improvements #1056

Merged
merged 11 commits into from
Mar 13, 2023

Conversation

Zhou-Ziheng
Copy link
Contributor

@Zhou-Ziheng Zhou-Ziheng commented Jan 28, 2023

There are certain types that needs to be improved as part of the Enterprise TypeScript conversion.
This PR aims to contain all changes that needs to be made.

Fixes #1122
BREAKING CHANGE: Selector Type removed from redux

@Zhou-Ziheng Zhou-Ziheng self-assigned this Jan 28, 2023
@codecov
Copy link

codecov bot commented Jan 28, 2023

Codecov Report

Merging #1056 (b958c9d) into main (a60bda5) will decrease coverage by 0.01%.
The diff coverage is 43.75%.

@@            Coverage Diff             @@
##             main    #1056      +/-   ##
==========================================
- Coverage   43.46%   43.46%   -0.01%     
==========================================
  Files         437      437              
  Lines       32951    32954       +3     
  Branches     8305     8309       +4     
==========================================
  Hits        14323    14323              
- Misses      18579    18582       +3     
  Partials       49       49              
Flag Coverage Δ
unit 43.46% <43.75%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/chart/src/ChartModelFactory.ts 76.92% <ø> (ø)
packages/code-studio/src/main/WidgetUtils.ts 5.55% <0.00%> (ø)
...udio/src/settings/ColumnSpecificSectionContent.tsx 98.72% <ø> (-0.01%) ⬇️
...e-studio/src/settings/FormattingSectionContent.tsx 63.11% <ø> (ø)
.../dashboard-core-plugins/src/ChartBuilderPlugin.tsx 0.00% <0.00%> (ø)
packages/dashboard/src/layout/LayoutUtils.ts 13.94% <0.00%> (ø)
packages/iris-grid/src/IrisGridUtils.ts 54.09% <ø> (ø)
packages/chart/src/ChartUtils.ts 64.19% <42.85%> (-0.23%) ⬇️
packages/code-studio/src/redux/selectors.ts 50.00% <50.00%> (ø)
...ackages/code-studio/src/settings/SettingsUtils.tsx 92.10% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mofojed
Copy link
Member

mofojed commented Jan 28, 2023

If you want to make sure this doesn't get merged, you can click "Convert to Draft" under the reviewers section to mark it as a draft.

@Zhou-Ziheng Zhou-Ziheng marked this pull request as draft January 29, 2023 01:43
@mofojed
Copy link
Member

mofojed commented Mar 2, 2023

@Zhou-Ziheng can you rebase this off the latest and mark this as "Ready for review" if it is ready for review?

@Zhou-Ziheng Zhou-Ziheng changed the title TypeScript Type Improvements refactor: TypeScript Type Improvements Mar 2, 2023
@Zhou-Ziheng Zhou-Ziheng requested a review from mofojed March 2, 2023 15:54
@Zhou-Ziheng Zhou-Ziheng marked this pull request as ready for review March 2, 2023 15:54
@@ -16,7 +16,7 @@ export type FormatOption = {
export type FormatterItem = {
columnType: string;
columnName: string;
format: Partial<TableColumnFormat>;
format: TableColumnFormat | Record<string, never>;
Copy link
Member

Choose a reason for hiding this comment

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

I think would be cleaner to do Partial<TableColumnFormat>

packages/components/src/navigation/MenuItem.tsx Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGridUtils.ts Outdated Show resolved Hide resolved
@mofojed
Copy link
Member

mofojed commented Mar 2, 2023

@Zhou-Ziheng also update the PR description to put "Fixes #1122" in the description, and change the Breaking notice to match the format we're expecting (e.g. "BREAKING CHANGE: Selector type removed from redux"

@Zhou-Ziheng Zhou-Ziheng requested a review from mofojed March 7, 2023 20:36
Comment on lines 96 to 97
item: FormattingRule & { id?: number; isNewRule?: boolean }
): FormattingRule {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like what's going on here with removeFormatRuleExtraProps and isFormatRuleValidForSave.
Instead, we should define a ValidFormatterItem type as well, so something like:

export type ValidFormatterItem = FormattingRule & {
  id?: number;
  isNewRule?: boolean;
};

export type FormatterItem = ValidFormatterItem & {
  format: Partial<TableColumnFormat>;
};

So it makes it a little more clear what constitutes an "invalid" FormatterItem.
Then these functions just become:

function removeFormatRuleExtraProps(item: ValidFormatterItem): FormattingRule;

function isFormatRuleValidForSave(rule: FormatterItem): rule is ValidFormatterItem;

I think that makes it a little clearer what's going on.

@Zhou-Ziheng Zhou-Ziheng requested a review from mofojed March 11, 2023 01:11
@Zhou-Ziheng Zhou-Ziheng merged commit 0be0850 into deephaven:main Mar 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve redux types
2 participants