plu / Pithub

Perl Github v3 API
http://metacpan.org/module/Pithub
Other
68 stars 36 forks source link

#186 breaks compatibility #187

Closed shogo82148 closed 8 years ago

shogo82148 commented 8 years ago

186 is great change, but it breaks compatibility.

My code doesn't work with Pithub v0.01030.

I think this feature should be optional.

schwern commented 8 years ago

Could you show an example of the code which broke, please?

shogo82148 commented 8 years ago

For example, following code shows the title of https://github.com/shogo82148/wikitest/pull/1

use warnings;
use strict;
use utf8;
use Encode;
use Pithub;

binmode(STDOUT, ":utf8");

my $pr = Pithub->new->pull_requests->get(
    user            => 'shogo82148',
    repo            => 'wikitest',
    pull_request_id => 1,
)->content;

my $title = decode_utf8($pr->{title});

print "$title\n";

It works with Pithub@0.01029

$ perl pithub.pl
add 申 and 月

but, it doesn't work with Pithub@0.01030

$ perl pithub.pl
Cannot decode string with wide characters at /home/ichinose/.plenv/versions/5.20.2/lib/perl5/5.20.2/x86_64-linux/Encode.pm line 215.
oalders commented 8 years ago

This would be the commit which changed the behaviour: https://github.com/plu/Pithub/commit/13e530232a09a95419d411334187c1814f6244dc

@shogo82148 if you remove the decode_utf8 call, does that solve the issue for you?

shogo82148 commented 8 years ago

if you remove the decode_utf8 call, does that solve the issue for you?

yes. it is very simple example, so it is very easy to remove extra decode_utf8 and add missing encode_utf8. But my code is too large to be resolved in this way.

I want a compatibility option. For example,

Pithub->new->utf8(0); # disable utf8 en/de-coding (default)
Pithub->new->utf8(1); # enable utf8 en/de-coding
oalders commented 8 years ago

That seems reasonable to me. Anyone else have an opinion on this? I'd like to see the decoding on by default, but if we consider this a breaking change (which it is in your case) then having it off by default makes sense too.

schwern commented 8 years ago

I would prefer UTF8 was on by default. UTF8 is more and more the norm. We should take the compatibility hit.

oalders commented 8 years ago

@shogo82148 Would you like to send a pull request implementing what you've described above, but with UTF8 on rather than off by default?

shogo82148 commented 8 years ago

@oalders I will.

I would prefer UTF8 was on by default.

I agree about UTF-8 on by default. but I also want to UTF-8 off by option.

oalders commented 8 years ago

@shogo82148 I think we're all in agreement then. If you could put this together, that would be great!