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

[9.x] Remove reduceWithKeys and merge the functionality with reduce #35901

Merged
merged 7 commits into from
Jan 18, 2021

Conversation

mokhosh
Copy link
Contributor

@mokhosh mokhosh commented Jan 15, 2021

This moves reduce out of Collection and LazyCollection and puts it into EnumeratesValues as suggested by @JosephSilber.

The existing test for reduce has be adapted to contain asserts for both situations, with and without $key.

I've also moved tap one method down to be alphabetically correct.

@JosephSilber while I was checking the code in EnumeratesValues I got an idea. Can we make reduce higher proxy-able as well? I don't know how to do that now, but I'm willing to do it if you give me a little guidance.

@driesvints
Copy link
Member

@JosephSilber you didn't get pinged by GitHub yet because we haven't merged the CODEOWNER changes yet into master so pinging you manually here.

@driesvints
Copy link
Member

@mokhosh there's a merge commit in this commit history. Can you rewrite the history so it's clean?

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 15, 2021

Sure

@JosephSilber
Copy link
Member

JosephSilber commented Jan 15, 2021

This moves reduce out of Collection and LazyCollection and puts it into EnumeratesValues

This was just done for 8.x, which is periodically merged with master. No need to do it here separately.

I've also moved tap one method down to be alphabetically correct.

Should probably go in a separate PR.

Can we make reduce higher proxy-able as well?

We can't. Higher order proxies work by calling a method on each item, and returning its values. Since reduce needs an additional parameter (the $carry), I believe it can't ever work with higher order proxies.

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 16, 2021

This was just done for 8.x, which is periodically merged with master. No need to do it here separately.

There's no diff there actually with the 8.x PR.
I should just say "this removes reduceWithKeys", I thought people who see this now might be in a 8.x mindset so I explained a bit more.

Should probably go in a separate PR.

I think it's already merged in the 8.x PR, so should we just leave it? I'll keep this in mind for the future contributions.

We can't. Higher order proxies work by calling a method on each item, and returning its values. Since reduce needs an additional parameter (the $carry), I believe it can't ever work with higher order proxies.

You're right. Thanks.

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 17, 2021

@driesvints I'm not sure why git rebase isn't showing me the merge commit so I can squash it. I red and watched a dozen tutorials on rebase, but they all worked on simple examples with one branch and no merge commits.

Can you please guide me through this?

@gocanto
Copy link
Contributor

gocanto commented Jan 18, 2021

@driesvints I'm not sure why git rebase isn't showing me the merge commit so I can squash it. I red and watched a dozen tutorials on rebase, but they all worked on simple examples with one branch and no merge commits.

Can you please guide me through this?

@mokhosh
You could do this from your CLI

git checkout master
git pull
git checkout mokhosh:master
git pull
git rebase master -i

#edit vim CL view
pick `add foo`
s `merge commit to be removed`
pick testReduce`

then, you can choose the commit message you fancy better for those changes. 👍🏻

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 18, 2021

@gocanto Thanks for your reply. The third command gave me this error:

error: pathspec 'mokhosh:master' did not match any file(s) known to git

@gocanto
Copy link
Contributor

gocanto commented Jan 18, 2021

@gocanto Thanks for your reply. The third command gave me this error:

error: pathspec 'mokhosh:master' did not match any file(s) known to git

mokhosh:master should be your master branch reference. Whatever the name or source that might be! :)

@driesvints
Copy link
Member

@mokhosh I'd just check out a new branch and do the code changes anew as the current timeline is a bit messed up now. Then force push to your branch here.

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 18, 2021

@driesvints Thank you, and sorry for the trouble. I'll be cleaner next time.

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.

5 participants