Optional encoding option

[tarlepp]

No description provided.

[jirwin]

+1 once the build passes

[jirwin]

Actually -1 for now. We need to figure out a good story for heavy dependencies like this.

[tarlepp]

Yeah that node-icu-charset-detector need some stuff, didn't find any other solution for this though. Any ideas?

[ghost]

We've discussed this on IRC (##node-irc on freenode; come join us!)
and have evaluated the options that we can find. It looks like
node-icu-charset-detector is the least worst option (despite the
native dependencies). I experimented with this pull req on my
branch, and you need to add - sudo apt-get -y install libicu-dev
to the before_install array in .travis.yml. Doing that, I saw
that there are some style issues.

Can you please fix the style issues, and make this an optional
dependency (both in package.json and in the code), so that those
who don't have a C compiler / access to the native library can keep
using node-irc? Thanks!

On 01/08/2015 01:48 PM, Tarmo Leppänen wrote:

Yeah that |node-icu-charset-detector| need some stuff, didn't find
any other solution for this though. Any ideas?

—
Reply to this email directly or view it on GitHub
#278 (comment).

[tarlepp]

Made necessary fixes.

[jirwin]

@tarlepp Thanks! Can you move the node-icu-charset-detector dependency to the optionalDependencies section of package.json? You'll have to create it. Read more about it here: https://docs.npmjs.com/files/package.json.

I believe in your original PR you didn't require the new libraries unless an encoding setting was configured? I'd prefer to go back to this model so that these new libraries won't actually cause issues if the setting isn't specified.

[jirwin]

LGTM +1