Matching commands precisely.

mastercoms / SlackMC

Link Slack to Minecraft!

MIT License

62 stars 11 forks source link

Matching commands precisely. #7

Closed david50407 closed 9 years ago

david50407 commented 9 years ago

Only matching the command not whole message; Commands now sending to Slack with quote '```'; Actions (e.g. login, logout) now sending to Slack with italic; Normal messages now sending to Slack without mrkdwn parsing.

!!! Notice that I changed the version to 1.5.0. If you don't want it upgrade, please don't use auto merge on GitHub.

mastercoms commented 9 years ago

I'll have to review this one more. Again, thank you for contributing. It really helps me a lot. Some initial thoughts (if I am wrong about something, please correct me):

we actually want to only see if a command contains an item in the blacklist
is there a benefit to disabling markdown parsing?

I think we can add this one as 1.5.0, since we're probably going to update it all in one batch, but I'm not going to do auto merge anyway, so I can update my own code and change a bit of yours.

mastercoms commented 9 years ago

Wow, I'm really sorry that I haven't reviewed this at all, I've just been really busy. I will see if I can get to it this weekend.

david50407 commented 9 years ago

My thoughts are:

Arguments of some commands like /login and /msg, must be hidden causing some privacy problems.
Sending messages from Minecraft to Slack with Markdown parsing may cause some confusions. Take an example, the message from minecraft: (_(_( would be rendered to Slack like this ((( with the second ( in italic.

If these are mismatch with your initial thoughts, you can just skip this PR. :)

mastercoms commented 9 years ago

That sounds good.

mastercoms commented 9 years ago

Instead of using isAction, could we just add the _'s to the message itself?

send("_logged in_", event.getPlayer().getName(), true);

david50407 commented 9 years ago

Sure :-)