fix: unset row_limit when it's not a number #387
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/hev3g00f1 |
Codecov Report
@@ Coverage Diff @@
## master #387 +/- ##
=======================================
Coverage 26.30% 26.31%
=======================================
Files 192 192
Lines 5291 5293 +2
Branches 474 475 +1
=======================================
+ Hits 1392 1393 +1
Misses 3864 3864
- Partials 35 36 +1
Continue to review full report at Codecov.
|
| @@ -42,7 +42,7 @@ export default function buildQueryObject<T extends QueryFormData>(formData: T): | |||
| metrics: processMetrics(formData), | |||
| order_desc: typeof order_desc === 'undefined' ? true : order_desc, | |||
| orderby: [], | |||
| row_limit: Number(row_limit), | |||
| row_limit: !Number(row_limit) && row_limit !== 0 ? undefined : Number(row_limit), | |||
There was a problem hiding this comment.
if we're converting row_limit to a number, won't row_limit !== 0 always be true?
could this be something like
const numericRowLimit = Number(row_limit);
...
row_limit: row_limit == null || isNaN(numericRowLimit) ? undefined : numericRowLimit,There was a problem hiding this comment.
I'm catching the case where if row_limit = 0, we don't want to set it to undefined.
!Number(0) is true, but we don't want row_limit to be undefined
There was a problem hiding this comment.
maybe this should be Number(row_limit) though...
There was a problem hiding this comment.
i see your point, updated
|
My apologies, apparently one of my changes caused this problem. I'll try not to make breaking changes to |
|
I think ideally there would be some integration tests between the main repo + these packages, but I think that's likely easiest once the API is refactored and the app itself is using these client calls. |
| @@ -42,7 +43,7 @@ export default function buildQueryObject<T extends QueryFormData>(formData: T): | |||
| metrics: processMetrics(formData), | |||
| order_desc: typeof order_desc === 'undefined' ? true : order_desc, | |||
| orderby: [], | |||
| row_limit: Number(row_limit), | |||
| row_limit: row_limit == null || isNaN(numericRowLimit) ? undefined : numericRowLimit, | |||
There was a problem hiding this comment.
Should use Number.isNaN, instead of isNaN
There was a problem hiding this comment.
I'm fixing this in my test coverage PR: https://github.com/apache-superset/superset-ui/pull/390/files
🐛 Bug Fix
Recently Superset was changed to not allow row_limit to be null, it can only be unset or a number. This fixes the library to match it
to: @williaster @ktmud @villebro @kristw