Skip to content

Set default encoding to UTF-8 #315

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 30, 2017
Merged

Set default encoding to UTF-8 #315

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e2285b
Add a note about how to install optional charset converter dependencies
Throne3d Aug 14, 2017
ec56eca
Default ircOptions.encoding to utf-8 if node-irc can convert encodings
Throne3d Aug 14, 2017
0ef0f09
Improve config check and warning message around encodings
Throne3d Sep 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ import config from './config.json';
discordIRC(config);
```

When installing the library, you may encounter an error relating to the installation of `iconv` or `node-icu-charset-detector`.
These are optional dependencies which allow you to set the target encoding of messages sent to Discord, as detailed below in the README.
Without these dependencies and the relevant setting, messages that aren't sent in UTF-8 may be corrupted when copied to Discord.

## Configuration
First you need to create a Discord bot user, which you can do by following the instructions [here](https://github.com/reactiflux/discord-irc/wiki/Creating-a-discord-bot-&-getting-a-token).

Expand Down Expand Up @@ -94,6 +98,13 @@ The `ircOptions` object is passed directly to irc-upd ([available options](https

To retrieve a discord channel ID, write `\#channel` on the relevant server – it should produce something of the form `<#1234567890>`, which you can then use in the `channelMapping` config.

### Encodings

If you encounter trouble with some characters being corrupted from some clients (particularly umlauted characters, such as `ä` or `ö`), try installing the optional dependencies `iconv` and `node-icu-charset-detector`.
The bot will produce a warning when started if the IRC library is unable to convert between encodings.

Further information can be found in [the installation section of irc-upd](https://github.com/Throne3d/node-irc#character-set-detection).

## Tests
Run the tests with:
```bash
Expand Down
9 changes: 9 additions & 0 deletions lib/bot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,15 @@ class Bot {
...this.ircOptions
};

// default encoding to UTF-8 so messages to Discord aren't corrupted
if (!('encoding' in ircOptions)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!ircOptions.encoding) or if you really want to explicitly check that the property isn't set: !ircOptions.hasOwnProperty('encoding')

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (irc.canConvertEncoding()) {
ircOptions.encoding = 'utf-8';
} else {
logger.warn('Cannot convert message encoding; you may encounter corrupted characters with non-English text.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to link to the section in your node-irc README here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Done.

}
}

this.ircClient = new irc.Client(this.server, this.nickname, ircOptions);
this.attachListeners();
}
Expand Down