search

makamaka / JSON

perl implementation of JSON encoder/decoder
42 stars 40 forks source link

2.54: Support Cpanel::JSON::XS as other fast XS choice #8

Closed rurban closed 11 years ago

rurban commented 11 years ago

Support Cpanel::JSON::XS as other fast JSON XS choice. Cpanel::JSON::XS has a public bugtracker, is maintained by cPanel Inc, works also with 5.6.2 does not use common::sense at run-time, only during testing.

Also fix some deprecated names in the testsuite. Skip non-matching prototype warnings.

rurban commented 11 years ago

Had to change to to,from_json prototypes from $@ to $;$ to match the XS version and avoid prototype mismatch warnings.

schwern commented 11 years ago

Seconded. I would like to be able to use a fast JSON parser with a compatible interface without risking myself or my users interacting with MLEHMANN.

rurban commented 11 years ago

Thanks. We had some cPanel internal discussion, and we will rename the module to cPanel::JSON::XS and upload it on the CPANEL account to CPAN. There needs also some possible 5.6 utf8 issues to be sorted out first. Please wait until then.

On Tue, Apr 23, 2013 at 3:28 PM, Michael G. Schwern < notifications@github.com> wrote:

Seconded. I would like to be able to use a fast JSON parser with a compatible interface without risking myself or my users interacting with MLEHMANN.

— Reply to this email directly or view it on GitHubhttps://github.com/makamaka/JSON/pull/8#issuecomment-16884221 .

Reini Urban http://cpanel.net/ http://www.perl-compiler.org/

rurban commented 11 years ago

We sorted it out. Unfortunately the name has to stay as it is, even we dont like it. I already uploaded that name. mst also uploaded now his JSON chooser JSON::MaybeXS using our name Cpanel::JSON::XS and not JSON::XS. It can be merged as is.

tokuhirom commented 11 years ago

I don't agree to merge this patch. The main issue around JSON::XS was resolved. (I guess the main issue of the JSON::XS at Apr. 2013 is mlehmann does not fix the issue around hash randomization)

makamaka commented 11 years ago

I don't mind supporting Cpanel::JSON::XS. (though the turn to check is JSON::XS first)

The reasons:

What do you think about this, tokuhirom-san?

tokuhirom commented 11 years ago

Perl 5.6 support was dropped by toolchain. ref. https://github.com/Perl-Toolchain-Gang/toolchain-site/blob/master/lancaster-consensus.md I think there is no reason to support 5.6 in 2013 by JSON.pm.

tokuhirom commented 11 years ago

The modules trying to load installed modules are terrible. It makes huge bugs. Remember JSON::Any did make terrible issues.

makamaka commented 11 years ago

I see. In view of tokuhirom's comments and following chat pages I decided against the merge.

http://lingr.com/room/perl_jp/archives/2013/08/01

http://yancha.hachiojipm.org/quot?id=85614,85612,85608,85607,85605,85604,85602,85600,85599,85598,85597,85596,85595,85594,85593,85592,85590,85589,85588,85587,85586,85585,85584,85583,85582,85581,85580,85579,85574,85564,85563,85562,85561,85559,85558,85557,85556,85555,85553,85528

I hope that Cpanel::JSON::XS have in cooperation with JSON::XS and it becomes better.

Songmu commented 11 years ago

I write up my opinion summary. (I know this issue is already closed.)

The format of JSON, JSON::PP and JSON::XS are simple and clear.

I don't think JSON needs multi XS backend support. it mess up more complexity in code and is confusing.

XS backend is should be limited only one module.

JSON::XS may break temporary and Cpanel::JSON::XS also may break temporary too.

It it that, if JSON supports multi XS backend, Many CPAN modules depending on JSON cost more maintainaunce fee.

Support multi XS backend may cause someone feels convinience, but it is not good at JSON.pm which should be stable.