Skip to content

Conversation

@TomPiona
Copy link
Contributor

[x] Wrote test for feature
[x] Added changes in the Changelog section in README.md
[x] Bumped version number (please change if incorrect)

Changes proposed:
Addressing #305. In addition to the existing way of joining, now a list/array of columns can be passed in to join on.

A question on behavior and one on design:

  • Behavior: Right now, when a label in the second table does not match the label in the first, I have it drop the second column label and use the first. Pandas keeps both columns if the labels don't align, so I was wondering if there was a preference.
  • Design: I split join into two functions, moving most of the original function to _join and the new stuff to _multiple_join, but there's a lot of repetitive code. Would it be better to leave everything under join and use some more control statements?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 75.0% when pulling 2e64376 on TomPiona:master into 5eaeb4e on data-8:master.

### v0.11.0

- Added `join` for multiple columns.

Copy link
Member

Choose a reason for hiding this comment

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

We'll need to update this before merging.

@davidwagner
Copy link
Member

This looks very nice. I'm a little wary of the fact that so much code has been duplicated (copy-pasted) in both _join() and _multiple_join(). That looks dangerous from a maintenance point of view (I'm worried we'll modify one of them without remembering to make the same change to the other). Can we refactor this to avoid repeated code (DRY principle); or to make _join(self, column_label, other, other_label) check that other_label is a string and then call self._multiple_join([column_label], other, [other_label])?

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.

3 participants