Skip to content
New issue

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

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

Already on GitHub? # to your account

chore: cleanup as unknown conversion #19587

Merged
merged 6 commits into from
Apr 8, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Apr 7, 2022

SUMMARY

Clean up as unknown after monorepo

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Pass all CI

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

@@ -285,7 +285,8 @@ describe('buildQueryObject', () => {
datasource: '5__table',
granularity_sqla: 'ds',
viz_type: 'table',
url_params: null as unknown as undefined,
// @ts-ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

ignore is used for the test case.

Copy link
Member

Choose a reason for hiding this comment

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

I believe // @ts-expect-error is the correct syntax for testing with incorrect types

@@ -226,7 +226,7 @@ function QuerySearch({ actions, displayLimit }: QuerySearchProps) {
value: xt,
label: xt,
}))}
value={from as unknown as undefined}
value={{ value: from, label: from }}
Copy link
Member Author

Choose a reason for hiding this comment

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

bycatch: use object fill in Select value

@@ -235,7 +235,7 @@ function QuerySearch({ actions, displayLimit }: QuerySearchProps) {
name="select-to"
placeholder={t('[To]-')}
options={TIME_OPTIONS.map(xt => ({ value: xt, label: xt }))}
value={to as unknown as undefined}
value={{ value: to, label: to }}
Copy link
Member Author

Choose a reason for hiding this comment

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

same before

@@ -247,7 +247,7 @@ function QuerySearch({ actions, displayLimit }: QuerySearchProps) {
value: s,
label: s,
}))}
value={status as unknown as undefined}
value={{ value: status, label: status }}
Copy link
Member Author

Choose a reason for hiding this comment

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

same before

filterSets: {},
filters: {
Copy link
Member Author

Choose a reason for hiding this comment

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

useless fields

Comment on lines -374 to +378
const engineParamsCatalog = Object.keys(
const engineParamsCatalog = Object.entries(
extra_json?.engine_params?.catalog || {},
).map(e => ({
name: e,
value: extra_json?.engine_params?.catalog[e],
).map(([key, value]) => ({
name: key,
value,
Copy link
Member Author

Choose a reason for hiding this comment

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

same logical substitution

catalog: Record<any, any> | string;
catalog?: Record<any, any> | string;
Copy link
Member Author

@zhaoyongjie zhaoyongjie Apr 7, 2022

Choose a reason for hiding this comment

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

catalog is an optional field in engine params

@zhaoyongjie zhaoyongjie requested a review from a team April 7, 2022 12:38
@zhaoyongjie zhaoyongjie changed the title Cleanup asunknown chore: cleanup as unknown conversion Apr 7, 2022
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #19587 (2ef9b67) into master (44e3103) will increase coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #19587      +/-   ##
==========================================
+ Coverage   66.65%   66.68%   +0.02%     
==========================================
  Files        1680     1681       +1     
  Lines       64292    64309      +17     
  Branches     6564     6565       +1     
==========================================
+ Hits        42855    42885      +30     
+ Misses      19735    19731       -4     
+ Partials     1702     1693       -9     
Flag Coverage Δ
javascript 51.52% <50.00%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
...lugin-chart-echarts/src/Timeseries/transformers.ts 51.23% <ø> (ø)
...ontend/src/SqlLab/components/QuerySearch/index.tsx 73.07% <ø> (ø)
...set-frontend/src/dashboard/util/injectCustomCss.ts 69.23% <0.00%> (ø)
...c/views/CRUD/data/database/DatabaseModal/index.tsx 32.79% <0.00%> (ø)
...ntend/packages/superset-ui-core/src/color/utils.ts 100.00% <100.00%> (ø)
...frontend/src/views/CRUD/alert/AlertReportModal.tsx 52.08% <0.00%> (-0.30%) ⬇️
...ins/legacy-plugin-chart-partition/src/Partition.js 0.00% <0.00%> (ø)
...ins/legacy-plugin-chart-sankey/src/ReactSankey.jsx 0.00% <0.00%> (ø)
...s/legacy-plugin-chart-horizon/src/HorizonChart.jsx 0.00% <0.00%> (ø)
...acy-plugin-chart-paired-t-test/src/PairedTTest.jsx 0.00% <0.00%> (ø)
... and 9 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 44e3103...2ef9b67. Read the comment docs.

Comment on lines 77 to 89
export function addAlpha(color: string, opacity: number): string {
// coerce values so ti is between 0 and 1.
const rounded = Math.round(Math.min(Math.max(opacity || 1, 0), 1) * 255);
return color + rounded.toString(16).toUpperCase();
// opacity value should be between 0 and 1.
if (opacity > 1 || opacity < 0) {
throw new Error(
`The alpha channel should between 0 and 1, but got: ${opacity}`,
);
}
// the alpha value is between 00 - FF
const alpha = `0${Math.round(opacity * 255)
.toString(16)
.toUpperCase()}`.slice(-2);

return `${color}${alpha}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

bycatch: fix codes coverage.

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.

Thanks for making the code cleaner! One nit, other than that LGTM

@@ -285,7 +285,8 @@ describe('buildQueryObject', () => {
datasource: '5__table',
granularity_sqla: 'ds',
viz_type: 'table',
url_params: null as unknown as undefined,
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

I believe // @ts-expect-error is the correct syntax for testing with incorrect types

@zhaoyongjie zhaoyongjie merged commit 761d5c4 into apache:master Apr 8, 2022
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 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
# for free to join this conversation on GitHub. Already have an account? # 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.

3 participants