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] Add Str::join() helper #43559

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

LukeTowers
Copy link
Contributor

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 #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

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 #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 LukeTowers marked this pull request as ready for review August 5, 2022 06:50
@dennisprudlo
Copy link
Contributor

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.

@bonzai
Copy link
Contributor

bonzai commented Aug 5, 2022

#42548

@LukeTowers
Copy link
Contributor Author

@bonzai that PR doesn't seem that similar to this one unless I'm missing something. However #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?

@GrahamCampbell GrahamCampbell changed the title Add Str::join() helper [9.x] Add Str::join() helper Aug 5, 2022
@bert-w
Copy link
Contributor

bert-w commented Aug 5, 2022

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
Copy link
Contributor

bert-w commented Aug 6, 2022

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.

@LukeTowers
Copy link
Contributor Author

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 #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().

@driesvints
Copy link
Member

driesvints commented Aug 8, 2022

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

* 2 items will return: $item1 . $dyadicGlue . $item2
* 3+ items will return: $item1 . $glue . $item2 . $lastGlue . $item3
*/
public static function join(iterable $items, string $glue = ', ', string $lastGlue = ', and ', $dyadicGlue = ' and '): string
Copy link
Contributor

@stevebauman stevebauman Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Code doc @params and @return annotations are missing.
  • A return type is given (:string). No other methods in the Str class have a return type.
  • Confusing name. It implies joining items together (like implode()), but it creates its own template. Would probably be better named to imply that. Like Str::listify() or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code docs are redundant when proper type hints are already provided on the parameters and for the return type, all they do open the possibility that they will become out of date if future changes are made; thus they have been intentionally excluded in favour of the language level type hinting.

Just because none of the other methods have a return type set yet doesn't mean that new ones can't or shouldn't (they should IMHO).

The name is actually the best choice here, given that Arr::join() and Collection::join() both exist already and provide their parameters in the same order as this method while also performing similar functionality; this method just also provides the $diadicGlue parameter as well. listify() doesn't make much sense to me personally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but your PR will be rejected for not complying with Laravel’s style rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style guide does not make a statement one way or the other whether you must include PHPDoc comments at all times, even when they're redundant; but I appreciate your concern. Do you have a more specific reference where they are listed as required?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukeTowers everywhere else in the code. Please remove the scalar types and format the docblocks like in the rest of the framework.

@taylorotwell
Copy link
Member

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.

@LukeTowers
Copy link
Contributor Author

@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 added a commit to wintercms/storm that referenced this pull request Aug 13, 2022
LukeTowers added a commit to wintercms/storm that referenced this pull request Aug 13, 2022
@LukeTowers LukeTowers deleted the patch-1 branch August 25, 2022 03:32
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.

8 participants