You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/08/27 10:00:30 UTC
[GitHub] [superset] zhaoyongjie opened a new pull request #16482: fix: ensure array return value in DND selector
zhaoyongjie opened a new pull request #16482:
URL: https://github.com/apache/superset/pull/16482
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
closes: https://github.com/apache/superset/issues/16458
#### After
https://user-images.githubusercontent.com/2016594/131109770-2877692f-afa9-4e5a-b560-d4e108b3bd45.mov
#### Before
Uploading Before_ensure_array.mov…
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: https://github.com/apache/superset/issues/16458
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16482:
URL: https://github.com/apache/superset/pull/16482#issuecomment-907097100
# [Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16482](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (207e5ea) into [master](https://codecov.io/gh/apache/superset/commit/8adc31d14c3b711cad6eb08a5223e06ef5fb723b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8adc31d) will **decrease** coverage by `0.00%`.
> The diff coverage is `20.00%`.
> :exclamation: Current head 207e5ea differs from pull request most recent head 60bf10a. Consider uploading reports for the commit 60bf10a to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #16482 +/- ##
==========================================
- Coverage 76.71% 76.70% -0.01%
==========================================
Files 1002 1002
Lines 53680 53681 +1
Branches 6852 6853 +1
==========================================
- Hits 41180 41177 -3
- Misses 12263 12267 +4
Partials 237 237
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.02% <20.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ontrols/DndColumnSelectControl/DndColumnSelect.tsx](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL0RuZENvbHVtblNlbGVjdC50c3g=) | `46.98% <0.00%> (-0.58%)` | :arrow_down: |
| [...ols/DndColumnSelectControl/utils/optionSelector.ts](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL3V0aWxzL29wdGlvblNlbGVjdG9yLnRz) | `46.15% <50.00%> (ø)` | |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `73.42% <0.00%> (-1.36%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8adc31d...60bf10a](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a change in pull request #16482: fix: can't drop column when name overlap
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699408545
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
##########
@@ -59,8 +59,9 @@ export function DndColumnSelect(props: DndColumnSelectProps) {
}
if (
typeof value === 'string' &&
- typeof optionSelectorValues === 'string' &&
- value !== optionSelectorValues
+ Array.isArray(optionSelectorValues) &&
+ optionSelectorValues.length === 1 &&
+ value !== optionSelectorValues[0]
Review comment:
Done. thanks
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
Sorry, I overlooked that part of the logic. fixed
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699201604
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
i think it's not really about compatibility between dnd and non-dnd, but what chart's logic expects - I think charts expect a string for a single value controls and an array for multi value controls. I tested this PR with Bubble Chart and I'm getting an error after changing a value in a non-multi control
CC @zhaoyongjie
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699201604
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
i think it's not really about compatibility between dnd and non-dnd, but what chart's logic expects - I think charts expect a string for a single value controls and an array for multi value controls. I tested this PR with Bubble Chart and I'm getting an error after changing a value in a non-multi control
CC @zhaoyongjie
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a change in pull request #16482: fix: can't drop column when name overlap
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699408545
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
##########
@@ -59,8 +59,9 @@ export function DndColumnSelect(props: DndColumnSelectProps) {
}
if (
typeof value === 'string' &&
- typeof optionSelectorValues === 'string' &&
- value !== optionSelectorValues
+ Array.isArray(optionSelectorValues) &&
+ optionSelectorValues.length === 1 &&
+ value !== optionSelectorValues[0]
Review comment:
Done. thanks
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
Sorry, I overlooked that part of the logic. fixed
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699103821
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
I assume the current return type `string[] | string | undefined` is there to make it fully compatible with the non-dnd control, which returns `string` when not in `multi` mode. Am I right @kgabryje?
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
##########
@@ -59,8 +59,9 @@ export function DndColumnSelect(props: DndColumnSelectProps) {
}
if (
typeof value === 'string' &&
- typeof optionSelectorValues === 'string' &&
- value !== optionSelectorValues
+ Array.isArray(optionSelectorValues) &&
+ optionSelectorValues.length === 1 &&
+ value !== optionSelectorValues[0]
Review comment:
I looked at `OptionSelector.has` - I agree that should probably be changed to something like
```typescript
has(value: string): boolean {
return ensureIsArray(this.getValues()).includes(value);
}
```
Would changing that solve this problem?
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
Right - I think v1 charts won't mind, but `viz.py` definitely has lots of logic that either assumes control values are either arrays or strings.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #16482: fix: can't drop column when name overlap
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16482:
URL: https://github.com/apache/superset/pull/16482#issuecomment-907097100
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699103821
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
I assume the current return type `string[] | string | undefined` is there to make it fully compatible with the non-dnd control, which returns `string` when not in `multi` mode. Am I right @kgabryje?
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
##########
@@ -59,8 +59,9 @@ export function DndColumnSelect(props: DndColumnSelectProps) {
}
if (
typeof value === 'string' &&
- typeof optionSelectorValues === 'string' &&
- value !== optionSelectorValues
+ Array.isArray(optionSelectorValues) &&
+ optionSelectorValues.length === 1 &&
+ value !== optionSelectorValues[0]
Review comment:
I looked at `OptionSelector.has` - I agree that should probably be changed to something like
```typescript
has(value: string): boolean {
return ensureIsArray(this.getValues()).includes(value);
}
```
Would changing that solve this problem?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #16482: fix: can't drop column when name overlap
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16482:
URL: https://github.com/apache/superset/pull/16482#issuecomment-907097100
# [Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16482](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (390b7ab) into [master](https://codecov.io/gh/apache/superset/commit/8adc31d14c3b711cad6eb08a5223e06ef5fb723b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8adc31d) will **decrease** coverage by `0.01%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #16482 +/- ##
==========================================
- Coverage 76.71% 76.69% -0.02%
==========================================
Files 1002 1002
Lines 53680 53690 +10
Branches 6852 6858 +6
==========================================
- Hits 41180 41178 -2
- Misses 12263 12275 +12
Partials 237 237
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.00% <0.00%> (-0.04%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ols/DndColumnSelectControl/utils/optionSelector.ts](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL3V0aWxzL29wdGlvblNlbGVjdG9yLnRz) | `46.15% <0.00%> (ø)` | |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `73.42% <0.00%> (-1.36%)` | :arrow_down: |
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `43.33% <0.00%> (-1.31%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `33.99% <0.00%> (-0.83%)` | :arrow_down: |
| [...dashboard/components/SliceHeaderControls/index.tsx](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMvaW5kZXgudHN4) | `75.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8adc31d...390b7ab](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie merged pull request #16482: fix: can't drop column when name overlap
Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged pull request #16482:
URL: https://github.com/apache/superset/pull/16482
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699103821
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
I assume the current return type `string[] | string | undefined` is there to make it fully compatible with the non-dnd control, which returns `string` when not in `multi` mode. Am I right @kgabryje?
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
##########
@@ -59,8 +59,9 @@ export function DndColumnSelect(props: DndColumnSelectProps) {
}
if (
typeof value === 'string' &&
- typeof optionSelectorValues === 'string' &&
- value !== optionSelectorValues
+ Array.isArray(optionSelectorValues) &&
+ optionSelectorValues.length === 1 &&
+ value !== optionSelectorValues[0]
Review comment:
I looked at `OptionSelector.has` - I agree that should probably be changed to something like
```typescript
has(value: string): boolean {
return ensureIsArray(this.getValues()).includes(value);
}
```
Would changing that solve this problem?
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
Right - I think v1 charts won't mind, but `viz.py` definitely has lots of logic that either assumes control values are either arrays or strings.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699202370
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
Right - I think v1 charts won't mind, but `viz.py` definitely has lots of logic that either assumes control values are either arrays or strings.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie merged pull request #16482: fix: can't drop column when name overlap
Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged pull request #16482:
URL: https://github.com/apache/superset/pull/16482
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #16482: fix: ensure array return value in DND selector
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699201604
##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
return !!this.getValues()?.includes(value);
}
- getValues(): string[] | string | undefined {
+ getValues(): string[] | undefined {
+ // ensure an array type return value
if (!this.multi) {
- return this.values.length > 0 ? this.values[0].column_name : undefined;
+ return this.values.length > 0 ? [this.values[0].column_name] : undefined;
Review comment:
i think it's not really about compatibility between dnd and non-dnd, but what chart's logic expects - I think charts expect a string for a single value controls and an array for multi value controls. I tested this PR with Bubble Chart and I'm getting an error after changing a value in a non-multi control
CC @zhaoyongjie
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #16482: fix: can't drop column when name overlap
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16482:
URL: https://github.com/apache/superset/pull/16482#issuecomment-907097100
# [Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16482](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (207e5ea) into [master](https://codecov.io/gh/apache/superset/commit/8adc31d14c3b711cad6eb08a5223e06ef5fb723b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8adc31d) will **decrease** coverage by `0.00%`.
> The diff coverage is `20.00%`.
> :exclamation: Current head 207e5ea differs from pull request most recent head 390b7ab. Consider uploading reports for the commit 390b7ab to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #16482 +/- ##
==========================================
- Coverage 76.71% 76.70% -0.01%
==========================================
Files 1002 1002
Lines 53680 53681 +1
Branches 6852 6853 +1
==========================================
- Hits 41180 41177 -3
- Misses 12263 12267 +4
Partials 237 237
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.02% <20.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ontrols/DndColumnSelectControl/DndColumnSelect.tsx](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL0RuZENvbHVtblNlbGVjdC50c3g=) | `46.98% <0.00%> (-0.58%)` | :arrow_down: |
| [...ols/DndColumnSelectControl/utils/optionSelector.ts](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL3V0aWxzL29wdGlvblNlbGVjdG9yLnRz) | `46.15% <50.00%> (ø)` | |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/16482/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `73.42% <0.00%> (-1.36%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8adc31d...390b7ab](https://codecov.io/gh/apache/superset/pull/16482?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org