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

fix: logic for 2D JSON data detection #840

Merged
merged 4 commits into from
Oct 4, 2021
Merged

Conversation

jkaster
Copy link
Contributor

@jkaster jkaster commented Sep 30, 2021

Only truly 2D JSON data should have a grid. This fixes erroneous DataGrid rendering

@github-actions
Copy link
Contributor

APIX Tests

    1 files    77 suites   2m 57s ⏱️
293 tests 280 ✔️ 13 💤 0 ❌
312 runs  299 ✔️ 13 💤 0 ❌

Results for commit bde6a98.

Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

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

Got a bunch of questions to answer before approving

* @param json to analyze
*/
const getTypes = (json: any) => {
const types = [new Set<ItemType>(), new Set<ItemType>()]
Copy link
Collaborator

Choose a reason for hiding this comment

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

can json be a string, number, boolean or date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the value passed in that we expect to be JSON could be any type

const data = json2Csv(content)
const showGrid = isColumnar(data.data)
const json = JSON.stringify(JSON.parse(response.body), null, 2)
const parsed = JSON.parse(response.body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if body is not JSON? parse will fail

Copy link
Contributor

Choose a reason for hiding this comment

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

the pickResponseHandler decides what renderer to use. If we're in this function it has to be a json:

https://github.com/looker-open-source/sdk-codegen/blob/main/packages/run-it/src/components/ShowResponse/responseUtils.tsx#L249-L259

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2021

APIX Tests

    1 files    77 suites   2m 12s ⏱️
293 tests 280 ✔️ 13 💤 0 ❌
312 runs  299 ✔️ 13 💤 0 ❌

Results for commit 6ccb3d8.

@jkaster jkaster requested a review from bryans99 October 1, 2021 21:41
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2021

APIX Tests

    1 files  ±0    77 suites  ±0   2m 10s ⏱️ -40s
294 tests +3  281 ✔️ +3  13 💤 ±0  0 ❌ ±0 
313 runs  +3  300 ✔️ +3  13 💤 ±0  0 ❌ ±0 

Results for commit dbbf919. ± Comparison against base commit eb1731f.

This pull request removes 1 and adds 4 tests. Note that renamed tests count towards both.
ShowResponse it renders json responses ‑ ShowResponse it renders json responses
ShowResponse it renders 2D json responses ‑ ShowResponse it renders 2D json responses
ShowResponse it renders no grid for one-row complex json ‑ ShowResponse it renders no grid for one-row complex json
responseUtils isColumnar considers a uniform array of objects a table ‑ responseUtils isColumnar considers a uniform array of objects a table
responseUtils isColumnar considers create_query json as complex ‑ responseUtils isColumnar considers create_query json as complex

Copy link
Contributor

@josephaxisa josephaxisa left a comment

Choose a reason for hiding this comment

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

lgtm, 🚢 it!

@jkaster jkaster merged commit 3d18b93 into main Oct 4, 2021
@jkaster jkaster deleted the jk/runit_2D_data_logic branch October 4, 2021 18:21
@github-actions

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants