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

Add tests for duplicate additional_variables/copy_variables in normal… #348

Merged
merged 1 commit into from Dec 14, 2018

Conversation

Projects
None yet
3 participants
@georgewambold
Copy link
Contributor

georgewambold commented Dec 13, 2018

…ize_entity

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Dec 13, 2018

CLA assistant check
All committers have signed the CLA.

@kmax12

This comment has been minimized.

Copy link
Member

kmax12 commented Dec 13, 2018

even though this is caught now, it is sort of just an fortunate "accident" that we now check for duplicate columns later in the process of loading an entity.

I'd say it is still worth it to add a check in normalize_entity(), so that we can give a more specific error of "Duplicate columns in copy_variables" or "Duplicate columns in additional_variables" when the user does this.

Want to add that? the test cases look good though

@georgewambold

This comment has been minimized.

Copy link
Contributor

georgewambold commented Dec 14, 2018

Ah, ok-- Yeah I'm down to add the more specific errors.

@georgewambold georgewambold force-pushed the georgewambold:add_duplicate_columns_custom_error branch from 5b01742 to 4414ffe Dec 14, 2018

@kmax12

This comment has been minimized.

Copy link
Member

kmax12 commented Dec 14, 2018

Looks good, merging. Thanks for the contribution, we appreciate it!

@kmax12 kmax12 merged commit d0ebff1 into Featuretools:master Dec 14, 2018

1 check passed

license/cla Contributor License Agreement is signed.
Details

jeff-hernandez added a commit to jeff-hernandez/featuretools that referenced this pull request Dec 14, 2018

@rwedge rwedge referenced this pull request Dec 17, 2018

Merged

v0.5.0 #351

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment