-
Notifications
You must be signed in to change notification settings - Fork 86
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
refactor: dataframe join
params
#912
refactor: dataframe join
params
#912
Conversation
join
params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good improvement.
Any other changes still needed? |
None that I can see. I'll run the workflow and as long as it all passes, I'll merge it later today |
Thank you! |
It looks like we do have some problems in pytests |
@timsaucer Ah my bad, we need to add this at the end of the function, then it should run:
Can't add it now myself to test :( |
Thanks for the update to fix the CI. We talked on discord and it brought up another issue - this is a breaking change for anyone who is currently doing a join without specifying the |
Ok, I made the adjustment. Assuming CI passes I'll merge. |
Which issue does this PR close?
Rationale for this change
Many times join keys are the same in both dataframes, so a simple on suffices. Also join_keys are deprecated over left_on and right_on.
What changes are included in this PR?
Are there any user-facing changes?