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

[5.4] Add new conjoin() method to collection #19318

Merged
merged 5 commits into from
May 25, 2017
Merged

[5.4] Add new conjoin() method to collection #19318

merged 5 commits into from
May 25, 2017

Conversation

mikebronner
Copy link
Contributor

This function allows joining of a traversable object with the current collection, ignoring the keys. This is useful when stacking collections based on different models or sources to be used for a singular purpose, for example generating of combined lists based off of multiple collections without overwriting values of existing keys.

Internals discussion: laravel/ideas#568

@mikebronner mikebronner changed the title Add new conJoin() method to collection [5.4] Add new conJoin() method to collection May 24, 2017
@JosephSilber
Copy link
Member

  1. How is this different than the merge method, besides for mutating the original collection (which we shouldn't do).

  2. The only difference would be if you have string keys, which you haven't even added any tests for.

  3. You can already merge string keys by calling values on the original collection:

    collect(['foo' => 'bar'])->values()->merge(['foo' => 'baz']);

@mikebronner
Copy link
Contributor Author

mikebronner commented May 24, 2017

Using merge will overwrite existing values for existing keys. I haven't added tests for string keys, because the key is irrelevant, although I should probably add tests to prove that they are irrelevant. Also, I was returning the original object, because other methods did the same, and I wanted to keep with that.

Point taken on #3.

Please close if this PR is deemed irrelevant.

The aim was to provide an easy way to join collections without having to figure it out each time. using the approach in bullet 3 works, but has to be thought through each time, and isn't as obvious as other collection methods.

@mikebronner
Copy link
Contributor Author

Upon further thought in the matter, example 3 above would only satisfy a one-off, but not conJoining of multiple collections or traversibles where some have string keys and other have index keys. The index keys would always be overwriten.

I will update the unit tests to include string keys.

@mikebronner
Copy link
Contributor Author

I reviewed the other methods in the collection class again, and saw what you meant by the original collection not being mutated. In those instances where it returned $this it was inspecting or doing other non-mutating actions, and all the methods that did perform mutations always prevented the original from being mutated.

I made the appropriate changes. Thanks @JosephSilber.

@thecrypticace
Copy link
Contributor

Why are you using the camel-cased name "conJoin"? It seems rather strange seeing that "conjoin" is not a compound word. Also wouldn't "concat" be a more appropriate name?

@mikebronner
Copy link
Contributor Author

mikebronner commented May 25, 2017

@thecrypticace Open to naming suggestions. :) I originally had it as conjoin, then for some reason made it camel-case. I'm not sure about concat, as that seems to be used predominantly for strings. I'll remove the camel-casing. Thanks for the feedback. :)

@thecrypticace
Copy link
Contributor

For reference: concat is the name of the operation in javascript (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat?v=control) and ruby (https://ruby-doc.org/core-2.4.0/Array.html#method-i-concat) and probably several other languages.

@mikebronner
Copy link
Contributor Author

I was looking at the PHP method http://php.net/manual/en/internals2.opcodes.concat.php as well as Excel (which is not related, of course, but its the first thing that comes to my mind).

@thecrypticace
Copy link
Contributor

CONCAT it's not a method in PHP. It's a low-level opcode which PHP uses during compilation. These aren't exposed to the user unless you dump them via the command line or are working on PHP itself. Opcodes are used in compilers and interpreters to specify an operation that is performed. (more reading: https://en.wikipedia.org/wiki/Opcode)

@mikebronner
Copy link
Contributor Author

Ah, you're right, of course, thanks for correcting me on it not being a method. I just did a quick google search and just skimmed it. If @taylorotwell is open to merging this, I'll defer to him for the naming, and certainly encourage him to take concat() into consideration. :)

@mikebronner mikebronner changed the title [5.4] Add new conJoin() method to collection [5.4] Add new conjoin() method to collection May 25, 2017
@taylorotwell taylorotwell merged commit 963f778 into laravel:5.4 May 25, 2017
@taylorotwell
Copy link
Member

Renamed to concat.

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.

4 participants