refs #4626: Added readable_list() method to Text class.

[marklocker]

This pull request adds the following feature: http://dev.kohanaframework.org/issues/4626

[zombor]

This should be moved into a parameter of the readable_list() method.

[marklocker]

The requested change has been made.

[shadowhand]

This needs to either use Oxford commas or have a static flag for them. Look
up "Oxford comma Stalin and hookers" if you don't know what this means.

[marklocker]

Good call, I apologise for the US-English ignorance...

I'll add this in over the weekend.

[zeelot]

Why the loop at the top? Why not just throw the exception in a single loop if you really want to check for type? I don't even thing it matters enough to check the types in this method.

[evanpurkhiser]

This brace should be on a new line.

[marklocker]

I've just pushed some changes. I've got a number of thoughts remaining:

1. With regards to checking parameter values before continuing, I personally like this as it makes a publicly facing method completely bullet proof - it's impossible to miss-use it. I understand that this might not be a trait found in other Kohana methods though, so I guess that's a design decision to be made.
2. If we are to keep the parameter value checking, I'm not sure whether a Kohana exception subclass must be used or if vanilla ones are acceptable. InvalidArgumentException seems to be the most suitable in this instance.
3. I've added serial comma support as an additional parameter. There's the possibility of making it somewhat dynamic based on the current locale setting. However, that would only really be useful if it can be safely assumed that en_US would want it on and en_GB would want it off by default. I'm not sure how this is usually approached within the project? For now, it's set as off by default, which is also up for debate.

[zeelot]

I believe we use the serial comma in other parts of our code and documentation so the default should probably be TRUE, I am still wondering why you have two loops though as it seems unnecessary. If you're going to throw an exception for invalid arguments, throw it in the one loop you have at the bottom.

The reason I would remove the checking is simply because the method should be usable with an array of objects that properly implement __toString(). Simple example would be an array of models.

[marklocker]

It's really just a logical style thing. There's no other reason than having a very clear separation between "validation" and processing. It's unlikely that you're going to want to output a readable list for a huge array, so performance wise I imagine the difference will be negligible. Regardless, it's not exactly unclear to be in the one loop, and it is more efficient, so I've merged the two.

I've also changed the exceptions to be instances of InvalidArgumentException and added an extra check to cater for objects implementing __toString(). I hadn't thought of that one - good call!

[zeelot]

Now (is_object($word) AND is_string((string)$word))) will probably throw an exception since you're trying to cast objects to a string with the purpose of checking if the object can be turned into a string ;P if it can't, it will be an exception "Catchable fatal error: Object of class Hello could not be converted to string on line 9". You probably have to check if the object has a __toString() method… it all seems a bit excessive still to me.

Also you need a space between the (string) cast.

[marklocker]

For some reason my brain must have been passively thinking that casting an object that doesn't implement __toString() would return FALSE or something! Don't ask why - it was late!

I've fixed it now, and I don't particularly think it's excessive...

[birkir]

Why all this mess while you can just copy and unset last item, implode array with comma, add conjunction with last item.

$last = $words[$sum - 1];
unset($words[$sum - 1]);
return impode(', ', $words).' and '.$last;

[cs278]

Or use array_pop().

[evanpurkhiser]

I agree, using array_pop and implode would make a lot of sense here.

[marklocker]

Agreed and updated.

[zeelot]

can you remove this empty line?

[zeelot]

I promise those two are my last comments :D then I'll merge it in!

[marklocker]

I've removed the empty line...

Otherwise, those checks still need to be present. As an example, one of the values could be a bool, and that'd break the logic (probably without error, but that's even worse in my opinion).

You could of course check that it's not a bool, but I think it's safer to check what it can be (a string, int or object that supports __toString()).

[zeelot]

Fair enough, I'll merge this in as soon as I get a chance.