Skip to content

[Work in progress] Cpanel::JSON::XS: use Data::Bool #109

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

Closed
wants to merge 5 commits into from

Conversation

aferreira
Copy link

@aferreira aferreira commented May 8, 2018

I am opening a discussion for moving the boolean support of JSON modules (like JSON::PP, JSON::XS, and Cpanel::JSON::XS) to other module, which can be used as the common interfaces for other Perl modules which need booleans as objects and which are interested in interoperability.

I am working on trial version of Types::Bool and the latest release on CPAN is: https://metacpan.org/release/FERREIRA/Types-Bool-2.98005-TRIAL

Let me know what you think of that.

@rurban
Copy link
Owner

rurban commented May 8, 2018

Why Types::Bool and not JSON::Types::Bool?
Types::Bool sounds too generic to me for this particular problem.

EDIT: I see. Well, what do the other serializers say?
Cpanel::JSON::XS has a lot more overloaded ops for bools.

@Grinnz
Copy link

Grinnz commented May 8, 2018

Please be aware of existing efforts in this area. It would be counterproductive to introduce multiple solutions to the problem of having too many solutions :) http://blogs.perl.org/users/tinita/2018/05/my-report-of-the-perl-toolchain-summit-2018-in-oslo.html (scroll to "bool.pm draft")

@aferreira
Copy link
Author

@Grinnz Tina's report inspired me to put this together. See https://metacpan.org/pod/release/FERREIRA/Types-Bool-2.98003-TRIAL/lib/Types/Bool.pm#DESCRIPTION and perlpunk/bool-p5#2

@rurban I contacted JSON::PP and JSON::XS maintainers and I am waiting on their feedback. I think the extra overloads used by Cpanel::JSON::XS are mostly harmless (as far as I can see) though not ideal.

@rurban
Copy link
Owner

rurban commented May 10, 2018

I'm fine with this generalization, as long as most JSON modules do this. But I'm only the second in the chain, please persuade the core module maintainer first, I'll follow then. Don't know about schmorp, but he was the first to come up with it. The biggest problem is only if we can trust the core maintainers to do the right thing. They don't do in p5p, JSON::PP should be different. Let them discuss this first.

@rurban rurban self-assigned this May 10, 2018
@rurban rurban added this to the Wait for JSON to decide milestone May 10, 2018
@aferreira aferreira force-pushed the devel/std-bool branch 2 times, most recently from f194092 to 59d83e9 Compare May 10, 2018 18:07
@pali
Copy link
Contributor

pali commented Jul 27, 2018

Alternatively all problems with booleans can be handled by https://metacpan.org/pod/Cpanel::JSON::XS::Type. Moreover it solves also string/int problems.

use Cpanel::JSON::XS;
use Cpanel::JSON::XS::Type;

encode_json([1], [JSON_TYPE_BOOL]);
# '[true]'
encode_json([0], [JSON_TYPE_BOOL]);
# '[false]'

@pali
Copy link
Contributor

pali commented Nov 18, 2018

Anyway, what is purpose of Types::Bool in Cpanel::JSON::XS? Cpanel::JSON::XS has already support for JSON_TYPE_BOOL type as shown in previous example.

@rurban
Copy link
Owner

rurban commented Nov 18, 2018

I think he wants a common interfaces for other Perl modules which need booleans as objects and which are interested in interoperability. Good goal indeed.

I'm only worried about the particular bad implementation, and the need to add one more package to check against at runtime. I'm with Lehmann on this. His effort is enough.

@pali
Copy link
Contributor

pali commented Nov 18, 2018

I'm only worried about the particular bad implementation, and the need to add one more package to check against at runtime.

Exactly, same there. There are already more bool types/objects and some of them area already supported by other JSON* modules. So why new? And also it targets again only booleans. And not integers, floats and other types. Cpanel::JSON::XS::Type already supports all JSON types.

@aferreira aferreira changed the title [Work in progress] Cpanel::JSON::XS: use Types::Bool [Work in progress] Cpanel::JSON::XS: use Data::Bool Nov 18, 2018
@aferreira
Copy link
Author

Update: changing the issue title to reflect that Types::Bool has been renamed to Data::Bool.

@rurban The only thing new here is delegating the code to handle to boolean objects to Data::Bool. There is no new package / stash – booleans by Data::Bool belong to JSON::PP::Boolean package via the same stash aliasing trick by Marc Lehman in Types::Serialiser. (As a matter of fact, the patch could be using Types::Serialiser for almost the same effect.)

@pali This is compatible with the convention of JSON::PP::Boolean objects as booleans. It is just a tiny proposal to move this specific species of Perl booleans into a unification. ( Maybe the problem is that this is just too pythonista: to have the one true boolean module. :-/ )

@pali I am having enough trouble for anyone to consider this bite-sized change. I am thinking "Small is better" and "Complex proposals get rejected faster" (like your makamaka/JSON-PP#32).

See makamaka/JSON-PP#36 (comment) and makamaka/JSON-PP#36 (comment)

@pali
Copy link
Contributor

pali commented Nov 19, 2018

I am thinking "Small is better"

And this is the reason why we have there 4 different JSON boolean objects... This is exactly same situation as in https://xkcd.com/927/

Look, in makamaka/JSON-PP#32 (comment) is just I don't think this is a good idea at all, but author did not proposed any alternative solution. Next he wrote Use some better standardized module, but without specifying what is that standardized module.

Based on current status, that standardized module can be Cpanel::JSON::XS::Type as @rurban accepted and agreed with current design and implementation into Cpanel::JSON::XS.

So what Types::Bool brings? Or which problem it fixes?

@aferreira
Copy link
Author

@pali I don't know what you mean by "4 different JSON boolean objects" – the current JSON modules out there all use \0 and \1 blessed to JSON::PP::Boolean. They also support the naked scalar references \0 and \1 (as kind of a legacy concession and / or quick-and-dirty way to express booleans). This is only concerned with the first.

The purpose here is simple. It consists of two steps.

The first is to "relieve" JSON modules from their boolean-related code – that is why these pull requests are mainly deletions: https://github.com/rurban/Cpanel-JSON-XS/pull/109/files, https://github.com/makamaka/JSON-PP/pull/36/files, https://github.com/aferreira/cpan-Types-Serialiser/pull/1/files

After that, the second step is a consequence of the first: to have a good answer when someone asks: "What boolean objects should I use in my Perl code?" That is meant to reduce interoperability issues among modules that need booleans – those are recurrent requests, like msgpack/msgpack-perl#37

@pali
Copy link
Contributor

pali commented Nov 19, 2018

@aferreira:

I don't know what you mean by "4 different JSON boolean objects"

Boolean objects currently supported by Cpanel::JSON::XS. They are:
JSON::PP::true, JSON::XS::true, Cpanel::JSON::XS::true, Types::Serialiser::true

Plus also it support references to 0 and 1.

And now you want that Cpanel::JSON::XS would support also Types::Bool::true. Why are not 4 types of booleans (+references) enough?? This is really https://xkcd.com/927/.

"What boolean objects should I use in my Perl code?"

In Perl code use Perl logic. Standard Perl logic is simple. False is undef, 0 and ''. And values 0 and 1 are used for booleans in most perl/cpan code. Exception is just this JSON hell and invention of fifth boolean does not make JSON hell better. Plus it does not solve problem with serializing JSON numbers...

Guys, sit down and think about it again. This invention of new Types::Bool::true does not help anything. People just start asking, what to use for JSON? Why not JSON::PP::true (which work now for every JSON module)? Or why not Types::Serialiser::true? How it would work with Moose Bool attributes? And why internal perl functions (like exists, defined, ...) which returns boolean value returns something different as Types::Bool? Why not to use standard Perl logic for booleans? And why not Perl model where operators (and functions) itself decide how to interpret variable -- and not variable decide how operators/functions evaluate them?

@rurban: I agree with you:

I'm only worried about the particular bad implementation, and the need to add one more package to check against at runtime. I'm with Lehmann on this. His effort is enough.

I do not see any value for adding new dependency... and Lehmann's solution is really enough.

@aferreira
Copy link
Author

And now you want that Cpanel::JSON::XS would support also Types::Bool::true.

It does not need to. Because it already does. There is no new stash – it is just "JSON::PP::Boolean". Notice that with the changes in this PR, all (unchanged) tests still pass.

As I put it before, the benefit is more like a social collaboration – (1) it works with the past code, (2) no need for every one to repeat the same boolean-related mambo jambo, (3) anyone that does not want to reinvent the wheel is invited to adopt a single module for their booleans.

Perl has no native representation for booleans. Most of the time the Perl concept of truth is enough. But when dealing with serialization of formats which support booleans, it is desirable to keep the booleans intact on round trips, eg. when writing after loading. And there are other good reasons for that, like strict validation via various mechanisms, like schemas, OpenAPI, type hierarchies, etc.

The problem with that was the coupling with JSON::PP for no apparent good reason. Booleans are independent of JSON and this association makes little sense when loading documents in formats like YAML, MessagePack, BSON, etc. However, the integration of the concept of boolean for all these applications is quite convenient.
-- https://metacpan.org/pod/Data::Bool#DESCRIPTION

@pali
Copy link
Contributor

pali commented Nov 19, 2018

There is no new stash – it is just "JSON::PP::Boolean". Notice that with the changes in this PR, all (unchanged) tests still pass.

This is just an implementation detail which is irrelevant when talking, describing or documenting public API. Also, most of users are interested in API, not implementation details (like optimization for converting C char* to int and etc...).

In fact your proposed API change for Cpanel::JSON::XS is adding support also Types::Bool::true object. Therefore it is adding fifth boolean type.

@rurban may decide at any time to fully or partially rewrite Cpanel::JSON::XS (without changing API.) E.g. to optimize that module or for any other reason. And when that happen all those boolean types need to be supported for backward compatibility.

As I put it before, the benefit is more like a social collaboration – (1) it works with the past code, (2) no need for every one to repeat the same boolean-related mambo jambo, (3) anyone that does not want to reinvent the wheel is invited to adopt a single module for their booleans.

How it differs from JSON::PP::Boolean or Types::Serialiser modules, which are already widely used?

Booleans are independent of JSON and this association makes little sense when loading documents in formats like YAML, MessagePack, BSON, etc.

For data serialization, basically every current solution just cause problems. You should look @choroba's YAPC talk about it: https://youtu.be/E70b73KlP6E

How is your module solving those serialization problems with scalars returned by perl functions or by any random cpan module (which obviously do not use Types::Bool) or module which use Moose/Moo/... Bool attribute?

@aferreira
Copy link
Author

Closing since it hasn't attracted the necessary interest.

@aferreira aferreira closed this Nov 27, 2018
@aferreira aferreira deleted the devel/std-bool branch May 25, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants