framework icon indicating copy to clipboard operation
framework copied to clipboard

[9.x] Add Str::join() helper

Open LukeTowers opened this issue 1 year ago • 7 comments

This adds the Str::join() helper that joins together a list of strings using different glue strings depending on how many items there are and what the current item's index is.

By default, it will take a list of items and join them together into a string that follows the Oxford Comma convention (i.e. "item 1, item 2, and item 3") but can be called to not use Oxford Commas (i.e. "item 1, item 2 and item 3") as well as handling lists with just one or two items total. Note that this is similar to Collection::join() but it allows for the specification of the diadic glue string separately and also operates on more than just collection instances. See https://github.com/laravel/framework/pull/27723 for the discussion around adding Collection::join()

It works on any iterable variable where each iteration can be cast to a string.

For more information on the Oxford Comma, see https://en.wikipedia.org/wiki/Serial_comma

LukeTowers avatar Aug 05 '22 06:08 LukeTowers

Wouldn't it be better to sync those function to prevent confusion or just change the join function on the collection to support Oxford Comma convention and use Str::join as an alias executing:

(new Collection($items))->join($glue, $lastGlue, $dyadicGlue);

Then there'd be only one single source of truth for joining items into a string.

dennisprudlo avatar Aug 05 '22 08:08 dennisprudlo

#42548

bonzai avatar Aug 05 '22 14:08 bonzai

@bonzai that PR doesn't seem that similar to this one unless I'm missing something. However https://github.com/laravel/framework/pull/42197 is interesting to me, perhaps it would be better if I instead moved my method's logic to Arr::join instead; although I prefer my approach where it can handle any iterable, not just arrays and not just collections which in my mind makes it make more sense for it to live in Str::join().

@driesvints what do you think about us merging this in and then changing Collection::join() and Arr::join() to use Str::join() internally?

LukeTowers avatar Aug 05 '22 17:08 LukeTowers

the Str class acts on strings, not on arrays. But the PR for Arr got denied as well. You can use collect(['a','b','c'])->join(',', ' and '); if you have a simpler usecase.

bert-w avatar Aug 05 '22 21:08 bert-w

Why don't you expand upon the Collection::join() function instead and rewrite so it can use your $dyadicGlue? Or if you desperately do not want it to be inside a collection, then add it as Arr::join() and have Collection::join() point to that instead.

bert-w avatar Aug 06 '22 15:08 bert-w

the Str class acts on strings, not on arrays. But the PR for Arr got denied as well. You can use collect(['a','b','c'])->join(',', ' and '); if you have a simpler usecase.

@bert-w the PR for Arr was accepted, see https://github.com/laravel/framework/pull/42197. However it falls short of being able to address the use case of serial (oxford) commas. As far as the Str class only operating on strings, there is some precedent set for operating on other values to produce strings (see createRandomStringsUsingSequence, replace, remove, substrReplace, and createUuidsUsingSequence).

I'll leave it up to @driesvints, would you prefer I move this logic to the Arr::join() and tweak Collection::join() to call it internally or are you fine with there being Str::join() as well? Personally I'm fine with either, although I have a slight preference for Str::join().

LukeTowers avatar Aug 06 '22 15:08 LukeTowers

Hey @LukeTowers. I don't review PR's myself, only Taylor does so he'll have to decide.

driesvints avatar Aug 08 '22 08:08 driesvints

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

taylorotwell avatar Aug 12 '22 17:08 taylorotwell

@taylorotwell would you prefer that I submit it as a PR to extend the functionality of the existing Arr::join() method and then also switch the Collection::join() method to use Arr::join() internally?

LukeTowers avatar Aug 12 '22 20:08 LukeTowers