Skip to content

Add column type information to table creation call#138

Merged
waxlamp merged 7 commits intomasterfrom
metadata-api-call
Nov 23, 2020
Merged

Add column type information to table creation call#138
waxlamp merged 7 commits intomasterfrom
metadata-api-call

Conversation

@waxlamp
Copy link
Copy Markdown
Contributor

@waxlamp waxlamp commented Nov 19, 2020

This PR makes use of multinet-app/multinetjs#22.

This uses the updated multinetjs library to send column type information at table upload time.

Depends on multinet-app/multinetjs#22 being merged and released.
Depends on #136.

@waxlamp waxlamp requested a review from jjnesbitt November 19, 2020 15:51
Comment thread src/utils/files.ts Outdated
Comment thread src/components/TableDialog.vue Outdated
Comment thread src/components/TableDialog.vue
@jjnesbitt
Copy link
Copy Markdown
Member

I think this looks really great! Just left a few suggestions

@jjnesbitt
Copy link
Copy Markdown
Member

Another suggestion I just thought of, which we'll likely want to do as a follow up:

Since we've now got this type selection UI, we could add a primaryKey type to our system, which would replace the following section in the first step:

Screenshot from 2020-11-19 14-39-59

We could remove this option from the first step, and just have it as another type in the column drop down in the second step. We would also likely default that to the _key field if it's there, or just the first column if it's not there.

There's important distinction from the rest of the types however, which is that there can only be one primary key column. We'd have to add some logic when it comes to selecting that type, and we'd need to create a new validation error on the server to handle multiple supplied primary keys, but those tasks seem fairly low-effort.

Since this involves both server and client changes, we should probably address it in a separate issue, and move forward with this PR. However, I thought it was worthy of discussion.

@waxlamp lmk what you think.

Co-authored-by: Jacob Nesbitt <jjnesbitt2@gmail.com>
@waxlamp
Copy link
Copy Markdown
Contributor Author

waxlamp commented Nov 19, 2020

Another suggestion I just thought of, which we'll likely want to do as a follow up:

Since we've now got this type selection UI, we could add a primaryKey type to our system, which would replace the following section in the first step:

Screenshot from 2020-11-19 14-39-59

We could remove this option from the first step, and just have it as another type in the column drop down in the second step. We would also likely default that to the _key field if it's there, or just the first column if it's not there.

There's important distinction from the rest of the types however, which is that there can only be one primary key column. We'd have to add some logic when it comes to selecting that type, and we'd need to create a new validation error on the server to handle multiple supplied primary keys, but those tasks seem fairly low-effort.

Since this involves both server and client changes, we should probably address it in a separate issue, and move forward with this PR. However, I thought it was worthy of discussion.

@waxlamp lmk what you think.

I think this is a good idea and I agree that we should address it in a followup. Can you an issue and link it back here?

Changes:
- `csvFileTypeRecommendations` -> `analyzeCSV` (function that now
  returns both a type recommendation *and* a table row sample)
- `Recommendation` -> `CSVAnalysis` (type encompassing the new return
  value above)
- `typeRecs` -> `analysis` (better name for the variable that gets the
  result of the analysis)
@waxlamp waxlamp requested a review from jjnesbitt November 19, 2020 21:37
jjnesbitt
jjnesbitt previously approved these changes Nov 19, 2020
Copy link
Copy Markdown
Member

@jjnesbitt jjnesbitt left a comment

Choose a reason for hiding this comment

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

While I approve of these changes, I'm holding off my official approval until the proper version of multinetjs is listed.

@jjnesbitt jjnesbitt dismissed their stale review November 19, 2020 21:51

Hold for multinetjs version bump

@waxlamp waxlamp requested a review from jjnesbitt November 20, 2020 20:48
Copy link
Copy Markdown
Member

@jjnesbitt jjnesbitt left a comment

Choose a reason for hiding this comment

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

:shipit:

@waxlamp waxlamp merged commit 2b88fa9 into master Nov 23, 2020
@waxlamp waxlamp deleted the metadata-api-call branch November 23, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants