search

TooTallNate / node-icy

Node.js module for parsing and/or injecting ICY metadata
MIT License
291 stars 48 forks source link

Optional `encoding` argument for #parse method #15

Closed romanenko closed 10 years ago

romanenko commented 10 years ago

Ability to parse non-utf8 metadata. Encoding should be provided as a second argument to #parse method. For example:

var output = icecast.parse(input, 'win1251');
TooTallNate commented 10 years ago

I'm not a huge fan of adding a new dependency for an optional feature. I think I'd rather have the user convert to a String explicitly, and/or I like the optional encoding idea, but it should only support the native built-in Node Buffer encodings (i.e. no iconv-lite).

I am interested in an API on top of the icecast.Reader and icecast.Writer classes that could set the encoding to use for subsequent icecast.parse() calls.

romanenko commented 10 years ago

Yes, I also think that adding a new dependency must be a reasonable decision. The problem was that build-in Node buffer encodings didn't have windows-1251 encoding, that was needed to parse metadata from some windows-based stream servers.

Is it a good idea to detect metadata encoding right in icecast.Reader, for example, by using some lib like chardet and convert everything to utf-8?

TooTallNate commented 10 years ago

I'd really rather leave that up to the user to decode non-standard encodings. Something like:

icecast.parse(iconv.decode(metadata, 'windows-1251'));
romanenko commented 10 years ago

Yes, this is a reasonable way. Then, I'm sure, we should close this PR and mention this approach as a possible solution for https://github.com/TooTallNate/node-icecast/issues/13

TooTallNate commented 10 years ago

Ok will do. Thanks for understanding and for the patch proposal!