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

chore: Removes unused vars #20194

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

michael-s-molina
Copy link
Member

SUMMARY

Removes unused vars to get rid of no-unused-vars warnings.

TESTING INSTRUCTIONS

1 - Execute all tests
2 - All tests should pass

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #20194 (7f0c604) into master (4674de1) will increase coverage by 0.15%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master   #20194      +/-   ##
==========================================
+ Coverage   66.32%   66.48%   +0.15%     
==========================================
  Files        1722     1737      +15     
  Lines       64639    64980     +341     
  Branches     6822     6896      +74     
==========================================
+ Hits        42874    43202     +328     
+ Misses      20031    20025       -6     
- Partials     1734     1753      +19     
Flag Coverage Δ
javascript 51.75% <66.66%> (+0.45%) ⬆️

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

Impacted Files Coverage Δ
...art-controls/src/operators/contributionOperator.ts 100.00% <ø> (ø)
...ui-chart-controls/src/operators/flattenOperator.ts 100.00% <ø> (ø)
...ui-chart-controls/src/operators/prophetOperator.ts 100.00% <ø> (ø)
...i-chart-controls/src/operators/resampleOperator.ts 100.00% <ø> (ø)
...et-ui-chart-controls/src/shared-controls/index.tsx 50.86% <ø> (+13.82%) ⬆️
...qlLab/components/EstimateQueryCostButton/index.tsx 5.26% <ø> (ø)
...frontend/src/SqlLab/components/SqlEditor/index.jsx 51.36% <ø> (+0.82%) ⬆️
...dashboard/components/SliceHeaderControls/index.tsx 64.28% <ø> (+0.43%) ⬆️
...ilters/FiltersConfigModal/FilterTitleContainer.tsx 79.41% <ø> (+22.26%) ⬆️
...nd/src/filters/components/TimeColumn/buildQuery.ts 0.00% <0.00%> (ø)
... and 157 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4674de1...7f0c604. Read the comment docs.

@zhaoyongjie
Copy link
Member

zhaoyongjie commented May 31, 2022

Removes unused vars to get rid of no-unused-vars warnings.

Some interfaces need to be redundant and stable so that changes in the function interface can be easily tracked. IMO, no-unused-vars should keep in.

@zhaoyongjie zhaoyongjie self-requested a review June 2, 2022 13:38
@zhaoyongjie zhaoyongjie dismissed their stale review June 2, 2022 13:39

this review need to disscuss

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

Some comments from offline discussion:


Every operator generator receives 2 arguments to generate operator, the interface of PostProcessingFactory is

export interface PostProcessingFactory<T> {
  (formData: QueryFormData, queryObject: QueryObject): T;
}

Even though we don’t use formData or queryObject in some operators, I suggest keeping the both arguments in function signature , if we remove them , each time a argument is added, then the prettier will reformat the codes

@michael-s-molina
Copy link
Member Author

michael-s-molina commented Jun 2, 2022

@zhaoyongjie I prefer to omit the parameters here just like any other function. Especially because it's clearly typed as PostProcessingFactory which allows the developer to easily check the available parameters. It's the same as a forEach where you have access to the value and index but often the index is omitted because it's not used.

Prettier will reformat the code the same way as it would for any other function. I don't think adding eslint ignores here is the way to go but curious to hear other opinions as well. @villebro @rusackas

@zhaoyongjie
Copy link
Member

zhaoyongjie commented Jun 2, 2022

Prettier will reformat the code the same way as it would for any other function. I don't think adding eslint ignores here is the way to go but curious to hear other opinions as well.

How do we avoid reformat codes and increase changes?

I think all of us often encounter changes to just one variable in a line, then change can become larger because of prettier to reformat. We should try to make these changes manageable, instead of making more and more changes.

@michael-s-molina
Copy link
Member Author

I really like the idea of reducing unnecessary changes but I don't think it's the case here. The variables are not being used, I don't think we should keep them to avoid a bigger diff during PR reviews. In fact, if one of these operators starts to use the arguments, then I would really like to see the signature change because it will clearly show that the variable is now being used.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Great to see this cleanup! However, I agree with @zhaoyongjie 's comments about the unified signature for the post processing operators (they should be kept consistent)

@michael-s-molina
Copy link
Member Author

michael-s-molina commented Jun 15, 2022

@zhaoyongjie @villebro I disabled the no-unused-vars lint rules for the operators to preserve the interface as requested.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-s-molina michael-s-molina merged commit d6f9fb5 into apache:master Jun 15, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants