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.

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

Move responsibility to compute dots and by to method #6559

Merged
merged 8 commits into from
Nov 30, 2022
Merged

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 23, 2022

Required for #6526.

Also fixes default argument of internal helper.

@krlmlr krlmlr requested a review from DavisVaughan November 23, 2022 09:30
@krlmlr krlmlr changed the title Fix default argument of internal helper Move responsibility to compute dots and by to method Nov 23, 2022
@DavisVaughan
Copy link
Member

@krlmlr do you still need this one? Right now filter_rows() and slice_rows() are rather symmetrical, which I like, so I'd rather only do this if we have to.

We can definitely fix the bad default arg either way, of course

@krlmlr
Copy link
Member Author

krlmlr commented Nov 30, 2022

Good catch, slice_rows() has the same problem, fixed that 🤪

I've added comments to the code to explain the reasoning behind the separation.

@DavisVaughan
Copy link
Member

@krlmlr would you please take a look at the CI?

@DavisVaughan
Copy link
Member

Actually I see the problem, I'll take over and manage merging, thanks!

@DavisVaughan DavisVaughan merged commit 367a1fa into main Nov 30, 2022
@DavisVaughan DavisVaughan deleted the b-default-arg branch November 30, 2022 21:16
@krlmlr
Copy link
Member Author

krlmlr commented Dec 1, 2022

Thanks for taking over, I pushed too hastily.

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