Set default encoding to UTF-8 #315
Conversation
Also warn when started if the IRC library cannot convert between encodings, in case users rely on this behavior.
|
I might be misunderstanding something here, but isn't utf8 already the default encoding in node-irc? https://github.com/Throne3d/node-irc/blob/master/lib/irc.js#L866-L868 If so I guess you'd only need the check and the warning message. |
lib/bot.js
Outdated
| if (irc.canConvertEncoding()) { | ||
| ircOptions.encoding = 'utf-8'; | ||
| } else { | ||
| logger.warn('Cannot convert message encoding; you may encounter corrupted characters with non-English text.'); |
There was a problem hiding this comment.
You might want to link to the section in your node-irc README here.
lib/bot.js
Outdated
| @@ -91,6 +91,15 @@ class Bot { | |||
| ...this.ircOptions | |||
| }; | |||
|
|
|||
| // default encoding to UTF-8 so messages to Discord aren't corrupted | |||
| if (!('encoding' in ircOptions)) { | |||
There was a problem hiding this comment.
if (!ircOptions.encoding) or if you really want to explicitly check that the property isn't set: !ircOptions.hasOwnProperty('encoding')
There was a problem hiding this comment.
The linter seems to complain about ircOptions.hasOwnProperty('encoding'):
.../discord-irc/lib/bot.js
95:21 error Do not access Object.prototype method 'hasOwnProperty' from target object no-prototype-builtins
I've made it use Object.prototype.hasOwnProperty.call(ircOptions, 'encoding') instead.
I think I decided to check for the property because if encoding is set without support for translating encodings, it will output errors for each message it attempts (and fails) to convert the encoding of. So, if the library erroneously says it can convert an encoding, and so discord-irc sets the encoding, (or if something else goes wrong along the way) it could be useful for people to manually disable the option by setting it to null.
There was a problem hiding this comment.
Also – what's the difference between 'encoding' in ircOptions and ircOptions.hasOwnProperty('encoding')? Whether the object ircOptions implements it itself, versus whether some superclass of it does?
There was a problem hiding this comment.
Yep: https://stackoverflow.com/questions/13632999/if-key-in-object-or-ifobject-hasownpropertykey
Probably doesn't really matter here though, but I think in general hasOwnProperty is considered a bit more hygienic.
1 similar comment
| @@ -91,6 +91,16 @@ class Bot { | |||
| ...this.ircOptions | |||
| }; | |||
|
|
|||
| // default encoding to UTF-8 so messages to Discord aren't corrupted | |||
| if (!Object.prototype.hasOwnProperty.call(ircOptions, 'encoding')) { | |||
There was a problem hiding this comment.
Hm, why did you use Object.prototype.hasOwnProperty instead of directly on ircOptions?
There was a problem hiding this comment.
The linter complained – I said in this comment.
Per #237, it would be good to default to using the UTF-8 encoding when the upstream IRC library is able to support this. There should also be some sort of mention in the README about how to ensure the bot can convert encodings.
This doesn't have tests for these lines. I'm not sure how to stub
irc.canConvertEncoding()except by including a new development dependency ofproxyquire, which is designed for specifically the purpose of stubbing requires on other files. Suggestions, comments?