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

Improve IRC → Discord mentions for non-word characters and partial word matches #273

Merged
merged 3 commits into from
Oct 27, 2017

Conversation

Throne3d
Copy link
Collaborator

Previously, attempting to mention someone with non-\w characters in their name would fail. So if you're on a server with a lot of non-latin names, or one where users have mysterious symbols in their names to indicate something (an example could be 'Throne✶', with the '✶' representing that one is an admin) then IRC users would not properly be able to ping them, as it wasn't recognized as being a wordy character. (Fortunately, if the user in question has 'Throne3d' as their Discord username, that worked as a fallback, but it was non-obvious. Typing the symbol would also be hard but plausibly could be copied and pasted.)

This builds on #272 (which standardizes the testing suite in ways that make this easier to test), adds case-insensitive username and nickname matching, matches all strings of characters that don't involve spaces or hash symbols instead of just \w ones, adds matching for mentions of the format @username#discriminator, and will match partially – e.g., if there's a user "Throne" in the channel, @Thronetest, @Throne's and @Throne-won't-see-this will all have the starting @Throne matched and turned into a mention, as Discord does with usernames.

The partial matching prefers longer matches first and foremost, case insensitively, and then prefers matches which have exact case. I couldn't find an easy method to compare case-insensitively across locales (it seems to require you list the locales you want to compare across), and I'm not sure what Discord does internally, so I uppercased them then compared (and have this in a separate function to make it easier to change later, if necessary).

There is, I think, duplicated logic, in that it checks for perfect non-partial matches first, but if that special casing were removed it should produce the same result (except with a bias towards exact case match, instead of what order the Collection uses behind the scenes), but I wasn't sure if there might be some sort of performance problem with large Discord servers so I thought it best to leave that there, at least for now.

@Throne3d Throne3d force-pushed the improve/mentions branch 2 times, most recently from 7666431 to 9faa90b Compare July 19, 2017 14:05
@Throne3d
Copy link
Collaborator Author

(rebased onto #272)

@qaisjp
Copy link

qaisjp commented Aug 15, 2017

I'm running this on my server. "Partial matching" is not the right description for "match by prefix".

For example, I'm "qaisjp".

  • @qaisjpRubbish now works
  • @qaisJP now works
  • partial matching implies that @qais would work (it doesn't)

In general though this is a great commit.

@Throne3d
Copy link
Collaborator Author

I phrased it that way because qaisJP would be a part of the 'word' qaisjpRubbish – admittedly it only works for prefix partial matches, so it's not in fact all partial matches, but I'd still consider that to be a partial match (one of the 'word', not the nickname).

Thanks.

@qaisjp
Copy link

qaisjp commented Aug 15, 2017

Hmm that's true.

In my mind I was thinking that "user input" partially matches "real nickname".

I guess you're saying "real nickname" partially matches "user input"?

@Throne3d
Copy link
Collaborator Author

Yeah. There's a partial match, in the "haystack" of the user input, of the "needle" of the actual username/nickname.

@Throne3d
Copy link
Collaborator Author

Throne3d commented Oct 5, 2017

(Rebased onto master and fixed a merge conflict.)

@Throne3d
Copy link
Collaborator Author

Throne3d commented Oct 22, 2017

@ekmartin Do you have time to review this? I was just thinking it would be nice to cut a new release with the few recent updates, and was going to ask you about it, but since there have been some new features (well, the ignore feature, plus some changes in the upstream irc-upd library), it would be nice if we could merge some of the other feature PRs before doing a minor version bump – specifically, this and #230 (which needs a rebase, but I'd be happy to do that, and I think it's addressed your requested changes), though some of the others might be useful too.

I mostly wonder this because, as far as I know, the current release uses an old version of irc-upd, which has an issue where the IRC library is permanently in debug mode.

It might also be worth regenerating the package-lock.json file, since it looks like it might be out of date. It seems to have old versions of some packages listed, e.g., irc-upd. (I'd do that by removing the node_modules folder, and the package-lock.json file, then re-running npm install, since it looks like sometimes Node updates it weirdly? I had trouble running nyc just doing a straight npm update.)

Edit: It looks like Travis is taking a while, and it might be due to something around package-lock.json. It also doesn't seem to test against Node 8. I'll open a pull request for this.

@ekmartin
Copy link
Member

Yeah - sorry about the delay. I'll set a reminder to get it done sometime this week.

@coveralls
Copy link

coveralls commented Oct 23, 2017

Coverage Status

Coverage increased (+0.1%) to 98.328% when pulling cff6eee on Throne3d:improve/mentions into 569893a on reactiflux:master.

@reactiflux reactiflux deleted a comment from coveralls Oct 23, 2017
@reactiflux reactiflux deleted a comment from coveralls Oct 23, 2017
@reactiflux reactiflux deleted a comment from coveralls Oct 23, 2017
@reactiflux reactiflux deleted a comment from coveralls Oct 23, 2017
@reactiflux reactiflux deleted a comment from coveralls Oct 23, 2017
@reactiflux reactiflux deleted a comment from coveralls Oct 23, 2017
@reactiflux reactiflux deleted a comment from coveralls Oct 23, 2017
@ekmartin ekmartin merged commit a45e07c into reactiflux:master Oct 27, 2017
@ekmartin
Copy link
Member

Good work :) I think it'd be nice to pull out some of the functionality from sendToDiscord into separate helper functions, but that can be done in separate PRs. Wanna test that everything still works in master and publish a release?

@Throne3d Throne3d deleted the improve/mentions branch October 27, 2017 17:55
@Throne3d
Copy link
Collaborator Author

Things seem to work fine in master! I'll get to making a changelog entry and minor version release.

@Throne3d
Copy link
Collaborator Author

Okay, new version released!

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

Successfully merging this pull request may close these issues.

None yet

4 participants