TO_JSON for Mojo::Collection

[wttw]

Adds Mojo::Collection::TO_JSON

The name is slightly misleading as it doesn't convert to JSON, rather it returns an array reference in the same way as to_array. This lets Mojo::JSON and friends convert a Mojo::Collection to a JSON array.

Amongst other things it reduces noise when returning Mojo::Pg result sets as JSON:

$c->render(json => {foo => $results->hashes});

vs

$c->render(json => {foo => $results->hashes->to_array});

[kraih]

Upper case methods should be at the top.

[kraih]

👎 from me i'm afraid. Like i said on IRC before, we've had a lot of bad luck with this kind of magic in Mojo::Collection, which makes me very sceptical of any new additions. There is also an inconsistency between Mojo::ByteStream and Mojo::Collection now, since both would behave differently when passed to JSON encoders like Cpanel::JSON::XS.

[kraih]

Just in case some of you don't remember those magical mistakes... we've had Mojo::Collection return true or false in boolean context depending on the number of elements, stringify all elements joined with newlines, and allowed arbitrary methods to be called on all elements with AUTOLOAD. And every single one of those turned out to be a huge problem.

[Grinnz]

I would be +1 if accompanied by a matching method for Mojo::ByteStream, and with the docs fixed up. I don't believe this is a problem like the other Mojo::Collection issues, as this only affects JSON encoding unlike overloading and AUTOLOAD, and treats the object as an arrayref and not an array.

[tempire]

It doesn't seem so bad to me, given that the "magic" is only what Mojo::JSON already does. I'm 👍, but open to being convinced otherwise.

[kraih]

This section needs to be moved to the top too.

[marcusramberg]

I'm 👎 for now because of the inconsistency it adds, I'm not as worried about the "magic"/convention of TO_JSON tho. I might be inclined to change my vote if Mojo::ByteStream got the TO_JSON method too.

[jhthorsen]

👍 from me. Same as @Grinnz's comments. I would actually stretch it to considering adding TO_JSON() to all the modules that does to_array or to_string, to make other mojo-objects consistent with Cpanel::JSON::XS.

I like throwing by nested objects on to Mojo::JSON and just let it figure out what to do, instead of traversing the objects twice to make sure all the array-ref-like objects are array-refs first.

[jberger]

Like @jhthorsen I think that many of the classes that implement to_string (and to_array, but there's only one of those) ought to have TO_JSON, except that I don't think they all should. Here is the list of those:

$ ack -l 'sub to_string'
lib/Mojo/ByteStream.pm
lib/Mojo/Cookie/Request.pm
lib/Mojo/Cookie/Response.pm
lib/Mojo/Cookie.pm
lib/Mojo/Date.pm
lib/Mojo/DOM.pm
lib/Mojo/Exception.pm
lib/Mojo/Headers.pm
lib/Mojo/Home.pm
lib/Mojo/Message.pm
lib/Mojo/Parameters.pm
lib/Mojo/Path.pm
lib/Mojo/URL.pm
lib/Mojolicious/Routes/Route.pm

While I can see the need to serialize several of these, I don't think we need to really concern ourselves with all of them (would someone really want to serialize a route?). That said, I'm now choosing between consistency and practicality, which I know will displease @kraih. I still remain neutral I suppose, though with some amount of sadness.

[kraih]

I have to strongly disagree with @jhthorsen, adding a TO_JSON method to classes like Mojo::Message would be a huge mistake.

[jhthorsen]

Sorry! Let me rephrase. I meant to make it consistent between Mojo::JSON and Cpanel::JSON::XS, I would add TO_JSON() to all the classes which does string overloading. I guess my to_array() statement still stand, since only Mojo::Collection has that method.

$ ack -l "to_string" lib/ | xargs ack -l '""' | xargs ack -h "^package"
package Mojo::Path;
package Mojo::Date;
package Mojo::Exception;
package Mojo::DOM;
package Mojo::ByteStream;
package Mojo::Parameters;
package Mojo::Cookie;
package Mojo::Home;
package Mojo::URL;

[kraih]

Ok, after some more discussions on IRC, i don't think this proposal is ready for voting yet. We first need to decide if we want Mojo::JSON to behave consistently (or to what degree) with Cpanel::JSON::XS.

[jberger]

Discussions (at length) on IRC have shown why it is significant to think of this case as distinct from the stringy ones, so on the issue of adding Mojo::Collection::TO_JSON I vote :+1:

[kraih]

Thanks, this pull request has been accepted by majority vote.