ContentWithMentionReplaced on roles and nicks (Fixes #208) #375

jD91mZM2 · 2017-05-04T18:46:33Z

Haven't tried if it works, so ¯\_(ツ)_/¯

iopred · 2017-05-04T19:03:13Z

message.go

-func (m *Message) ContentWithMentionsReplaced() string {
-	if m.Mentions == nil {
-		return m.Content
+func (m *Message) ContentWithMentionsReplaced(s *Session) (content string, err error) {


This is breaking the API, we should have a ContentWithMentionsReplacedWithSession and document that this will replace more data than above.

iopred · 2017-05-04T19:04:57Z

message.go

+		if !role.Mentionable {
+			continue
+		}
+		content = regexp.MustCompile("<@&"+regexp.QuoteMeta(role.ID)+">").ReplaceAllString(content, "@"+role.Name)


This is really unperformant, instead of iterating through all the roles on each message we should use regexp.ReplaceAllStringFunc instead here, search for <@&[.*?]> and then search for the correct role name only when a mention happens.

Well, I know it's slow and inefficient and crappy. I just kept the same syntax as previously used ¯\_(ツ)_/¯.

The difference is before the mentions array tells us which things are mentioned, in this case we don't know that theres a role mention, and so we're iterating through each role and doing a regex.

Oh... yeah that's true. Nice catch!

iopred

While you're here, you should also make channel mentions work too.

Also, a test here wouldn't hurt, should be very easy to create as we don't need Session to be connected.

iopred · 2017-05-08T13:50:20Z

message.go

+
+	channel, err := s.State.Channel(m.ChannelID)
+	if err != nil {
+		return


Should probably return ContentWithMentionsReplaced.

iopred · 2017-05-08T13:52:30Z

message.go

-	return content
+
+	if patternRoles == nil {
+		patternRoles = regexp.MustCompile("<&[^>]*>")


Only compile this once, where you define the var, also I prefer a lazy regex:

<&[0-9]*>

I do only compile this once. The difference is that I don't compile it in case it's not used

True, but what if Discord decides not to use 0-9 or something weird like that?

id's are actually numbers, we just use strings internally.

I know. But we never know if they decide to change them. Or, we do know that that's not gonna happen. Still. Better be paranoid 😛

Nope, being paranoid means you litter your code with defensiveness that is unwarranted.

Discord uses snowflakes as their ID's which are 64bit ints and have huge amounts of infrastructure around it. In fact we are actually the bad guys because we don't define our ID's as strings (which python does for example).

At worst you should be using (.*?), but [0-9]* is correct.

iopred · 2017-05-08T13:53:48Z

message.go

+			return mention
+		}
+
+		return "<@&" + role.ID + ">"


Don't you want return "@"+role.Name here?

iopred · 2017-05-08T14:02:20Z

message.go

+	}
+
+	patternRoles.ReplaceAllStringFunc(content, func(mention string) string {
+		role, err := s.State.Role(channel.GuildID, mention)


This won't work right? You're passing <&40394820394209> here as the role id instead of just 40394820394209, in bruxism I just do mention[3 : len(mention)-1]

Right, yeah, that's true. Will correct!

jD91mZM2 · 2017-05-08T14:14:17Z

It does require a session to get the channel 🤔

EDIT: I just realized there is MentionRoles. Should I use that?

iopred · 2017-05-08T14:23:40Z

Oh, there is! Yeah definitely use MentionRoles instead of the regexp, but use similar regexp code for MentionRooms.

You can make a fake Session/State:

fakeState = &State{}
fakeState.Add.....
fakeSession = &Session{State:fakeState}

message.ContentWithMoreMentionsReplaced(fakeSession)

jD91mZM2 · 2017-05-08T14:31:58Z

Yep, trying to fake a session and state already 😛

iopred · 2017-05-08T14:58:11Z

message.go

-	return content
+
+	if patternChannels == nil {
+		patternChannels = regexp.MustCompile("<#[^>]*>")


Compile this once.

I think you missed this, move this to line 183 and use "<#[0-9]*>"

iopred · 2017-05-08T15:02:48Z

message.go

+
+	for _, user := range m.Mentions {
+		content = strings.NewReplacer(
+			"<@"+user.ID+">", "@"+user.Username,


This should be user.Username:
https://discordapp.com/developers/docs/resources/channel#message-formatting

Only the @! replaces with nick.

Realized that, but didn't bother to fix it, because for some reason I thought I'd have to make a bunch of workarounds. Didn't realize it was as easy as changing that 😛

iopred · 2017-05-08T15:34:05Z

message.go

-		content = regexp.MustCompile(fmt.Sprintf("<@!?(%s)>", user.ID)).ReplaceAllString(content, "@"+user.Username)
+		member, err := s.State.Member(channel.GuildID, user.ID)
+		nick := user.Username
+		if (err == nil && member.Nick != "") || nick == "" {


I think the nick == "" check is unneeded, a Member will always have a Username.

That's true. No idea why I put that there 😆

iopred · 2017-05-08T15:34:21Z

Thanks for all the work on this guy <3

jD91mZM2 · 2017-06-13T12:56:51Z

Reason I didn't move it at first was because I thought only initiating it if necessary would be better. But I guess all global variables in Go are lazily initiated anyways, so... ¯_(ツ)_/¯

And then now I realized it could cause race conditions. Better just move it where it should be

iopred · 2017-06-13T15:49:15Z

Thanks again!

…) (bwmarrin#375) * ContentWithMentionReplaced on roles and nicks (Fixes bwmarrin#208) * Compatibility * Like this? 🤔 * More efficient regexp * Tweaked a little * Tweaked a little more * Tweaked even more * Removed unessecary crap condition that is useless * Disallow voice channel * Moved regexp declaration

ContentWithMentionReplaced on roles and nicks (Fixes #208)

e7a8d86

iopred reviewed May 4, 2017

View reviewed changes

LEGOlord208 added 3 commits May 4, 2017 21:13

Compatibility

f0d4ea1

Like this? 🤔

31f2602

More efficient regexp

d629fe3

iopred reviewed May 8, 2017

View reviewed changes

LEGOlord208 added 2 commits May 8, 2017 16:54

Tweaked a little

9fa35ff

Tweaked a little more

aeec0d0

iopred reviewed May 8, 2017

View reviewed changes

Tweaked even more

59c7746

iopred reviewed May 8, 2017

View reviewed changes

LEGOlord208 added 2 commits May 8, 2017 17:39

Removed unessecary crap condition that is useless

062f99d

Disallow voice channel

9113131

bwmarrin added this to the v0.17.0 milestone May 15, 2017

LEGOlord208 added 2 commits June 13, 2017 14:54

Merge branch 'develop' into 2

42db579

Moved regexp declaration

37e2334

iopred merged commit 5a02430 into bwmarrin:develop Jun 13, 2017

bwmarrin mentioned this pull request Jul 4, 2017

Message.ContentWithMentionsReplaced() replace role and channel mentions. #208

Closed

jD91mZM2 deleted the 2 branch July 23, 2017 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ContentWithMentionReplaced on roles and nicks (Fixes #208) #375

ContentWithMentionReplaced on roles and nicks (Fixes #208) #375

jD91mZM2 commented May 4, 2017 •

edited

Loading

iopred May 4, 2017

jD91mZM2 May 4, 2017

iopred May 4, 2017

jD91mZM2 May 4, 2017 •

edited

Loading

iopred May 4, 2017

jD91mZM2 May 4, 2017

iopred left a comment

iopred May 8, 2017

iopred May 8, 2017

jD91mZM2 May 8, 2017 •

edited

Loading

iopred May 8, 2017

jD91mZM2 May 8, 2017

iopred May 8, 2017 •

edited

Loading

iopred May 8, 2017

jD91mZM2 May 8, 2017

iopred May 8, 2017

jD91mZM2 May 8, 2017

jD91mZM2 commented May 8, 2017 •

edited

Loading

iopred commented May 8, 2017

jD91mZM2 commented May 8, 2017

iopred May 8, 2017

iopred May 8, 2017

iopred May 8, 2017

jD91mZM2 May 8, 2017

iopred May 8, 2017

jD91mZM2 May 8, 2017

iopred commented May 8, 2017

jD91mZM2 commented Jun 13, 2017

iopred commented Jun 13, 2017

ContentWithMentionReplaced on roles and nicks (Fixes #208) #375

ContentWithMentionReplaced on roles and nicks (Fixes #208) #375

Conversation

jD91mZM2 commented May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iopred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iopred May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 commented May 8, 2017 • edited Loading

iopred commented May 8, 2017

jD91mZM2 commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iopred commented May 8, 2017

jD91mZM2 commented Jun 13, 2017

iopred commented Jun 13, 2017

jD91mZM2 commented May 4, 2017 •

edited

Loading

jD91mZM2 May 4, 2017 •

edited

Loading

jD91mZM2 May 8, 2017 •

edited

Loading

iopred May 8, 2017 •

edited

Loading

jD91mZM2 commented May 8, 2017 •

edited

Loading