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.

Sign up for GitHub

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

Jump to bottom

[APIC-305] Require sql_type for every column when table_columns is provided #400

Merged
merged 15 commits into from
Aug 19, 2020
Merged

[APIC-305] Require sql_type for every column when table_columns is provided #400

merged 15 commits into from
Aug 19, 2020

Conversation

izzy-zelichenko
Copy link
Contributor

APIC-305

Ensure data types are detected when the destination table does not exist or it is being dropped and table_columns are provided but do not contain a sql_type for every column.

uttercm
uttercm suggested changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@uttercm uttercm left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of questions. We should also request a review from someone on team sparkle since this is their AOR

civis/io/_tables.py Outdated Show resolved Hide resolved
civis/io/_tables.py Outdated Show resolved Hide resolved
uttercm
uttercm suggested changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@uttercm uttercm left a comment

Choose a reason for hiding this comment

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

One comment about the check we are using to throw the error

civis/io/_tables.py Outdated Show resolved Hide resolved
@uttercm uttercm assigned izzy-zelichenko and unassigned uttercm Aug 18, 2020
jkulzick
jkulzick suggested changes Aug 18, 2020
View reviewed changes
civis/io/_tables.py Outdated Show resolved Hide resolved
civis/io/_tables.py Outdated Show resolved Hide resolved
civis/io/_tables.py Outdated Show resolved Hide resolved
civis/io/_tables.py Outdated Show resolved Hide resolved
civis/io/_tables.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Outdated Show resolved Hide resolved
@jkulzick jkulzick removed their assignment Aug 18, 2020
@izzy-zelichenko
- Update error description
b2407a6 
- Fix tests
- Use snake case
izzy-zelichenko
izzy-zelichenko commented Aug 18, 2020
View reviewed changes
civis/io/_tables.py Outdated Show resolved Hide resolved
izzy-zelichenko
izzy-zelichenko commented Aug 18, 2020
View reviewed changes
civis/io/_tables.py Outdated Show resolved Hide resolved
@izzy-zelichenko
Copy link
Contributor Author

We now have a test for every case when table_columns are provided

uttercm
uttercm suggested changes Aug 18, 2020
View reviewed changes
civis/io/_tables.py Outdated Show resolved Hide resolved
@uttercm uttercm assigned izzy-zelichenko and unassigned uttercm Aug 18, 2020
jkulzick
jkulzick suggested changes Aug 19, 2020
View reviewed changes
civis/io/_tables.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Outdated Show resolved Hide resolved
civis/tests/test_io.py Outdated Show resolved Hide resolved
@jkulzick jkulzick removed their assignment Aug 19, 2020
@izzy-zelichenko
- Use a more fitting variable name
45f5e0c 
- Remove duplicate test, add additional sql_type entry to test to be thorough
@izzy-zelichenko
Copy link
Contributor Author

I removed the test I previously added because I realized it was nearly identical to the test test_civis_file_to_table_table_doesnt_exist_provide_table_columns, so I just updated that existing test to have two table_columns provided to be more thorough.

jkulzick
jkulzick approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@jkulzick jkulzick left a comment

Choose a reason for hiding this comment

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

LGTM

uttercm
uttercm approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@uttercm uttercm left a comment

Choose a reason for hiding this comment

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

LGTM

@uttercm uttercm assigned izzy-zelichenko and unassigned uttercm Aug 19, 2020
@izzy-zelichenko izzy-zelichenko merged commit 8c8981e into master Aug 19, 2020
@izzy-zelichenko izzy-zelichenko deleted the require-sql-types branch August 19, 2020 17:04
@mheilman
Copy link
Contributor

@izzy-zelichenko, would you mind making a followup PR to add an item to the changelog for this?

@izzy-zelichenko
Copy link
Contributor Author

@mheilman Sure thing.

@izzy-zelichenko izzy-zelichenko mentioned this pull request Sep 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkulzick jkulzick jkulzick approved these changes

@uttercm uttercm uttercm approved these changes

Assignees

@izzy-zelichenko izzy-zelichenko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@izzy-zelichenko @mheilman @jkulzick @uttercm