Skip to content
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

Add encoding support #3

Closed
wants to merge 2 commits into from

Conversation

dagolden
Copy link

@dagolden dagolden commented Sep 8, 2013

  1. now possible to pick an encoding via the "use" statement

    use M:L:R -readers => { encoding => ... }

  2. default is "encoding(UTF-8)"

  3. (read|write)_file now take options hashref and allow 'encoding'

1. now possible to pick an encoding via the "use" statement

    use M:L:R -readers => { encoding => ... }

2. default is "encoding(UTF-8)"

3. (read|write)_file now take options hashref and allow 'encoding'
@dagolden
Copy link
Author

dagolden commented Sep 8, 2013

Addresses #1

@rjbs rjbs mentioned this pull request Sep 9, 2013
@rjbs
Copy link
Owner

rjbs commented Sep 9, 2013

I have reviewed this and I think it is all good. I will try to merge it at once with a few other changes.

@dagolden
Copy link
Author

As I reflect on @dolmen's original concern, perhaps the "encoding" option should be renamed "binmode" for clarity. (FWIW, that's the name I used for Path::Tiny.) Then people will get that ":raw" or ":utf8" or ":utf8_strict" or ":encoding(UTF-8)" are all legitimate.

@rjbs
Copy link
Owner

rjbs commented Sep 10, 2013

I prefer binmode also. I was going to suggest it once I saw your patch, but decided it wasn't worth worrying about. I'm glad somebody else mentioned it, because I like it better, too.

@dolmen
Copy link

dolmen commented Sep 10, 2013

👍 for binmode: this makes explicit that an I/O layer is expected instead of a bare encoding name.

@rjbs
Copy link
Owner

rjbs commented Oct 18, 2013

This is now in a trial release.

@rjbs rjbs closed this Oct 18, 2013
@haarg
Copy link
Contributor

haarg commented Oct 19, 2013

Doesn't this break 5.6 compatibility? The metadata should be updated to reflect that. Also, IO::String could be dumped in favor using PerlIO.

@rjbs
Copy link
Owner

rjbs commented Oct 19, 2013

I have added a prereq for 5.8.1

The IO::String, I'll look at doing a bit later, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants