Configurable formatting for irc action /me messages sent to Discord

[RheaAyase]

I'm not much of a js person myself, so please do provide feedback, if you need me to change, rename, move something...

[Throne3d]

Hi there! First of all, thanks for this. A few comments:

• It's a bit weird that regular messages are now routed through sendActionToDiscord and also that this now takes an isAction parameter. I'd suggest having the action event call sendActionToDiscord(author, to, text) and then having that, separately, call sendToDiscord(author, channel, text, true) (modifying sendToDiscord to take an isAction parameter). If this parameter is provided (as will be the case for sendToDiscord when it's not an action), the parameter is undefined which evaluates to false in a condition, so this should allow the rest of the code to work as expected.
• As the Travis build says, the changes break linting here because the line length is too long. I'd suggest extracting the ternary condition to make it shorter, changing the line to something like:

  const format = isAction ? this.formatDiscordMe : this.formatDiscord;
  const withAuthor = Bot.substitutePattern(format, patternMap);

• The default format for 'me' should be _${text}_, as it was before – can you change that here?
• Could you also add a test, approximately here (to bot.test.js near the other formatting tests), to make sure the bot obeys the custom formatting when given?

Let me know if you need any help with this, or if you'd prefer I make the changes myself.

[RheaAyase]

It's a bit weird that regular messages are now routed through sendActionToDiscord and also that this now takes an isAction parameter.

Sure it makes little sense, but the whole code around sending those messages made very little sense, hence I just placed it at the first place where i could, without changing too much.

...this was done this way to not have to rename it everywhere, as I'm using vim and not an editor that would do that easily. Hence keeping all the old references without breaking them - not knowing the language. (Overloading the method instead of using default parameter - I was now told that this should be fine so I'll do that.)

because the line length is too long

lol

The default format for 'me' should be ${text}, as it was before

And here we will disagree. IRC (any good IRC clien?) has /me formatting with a star, followed by "name did something" without what you would insert, the **<name>** _something_

...so anyway, that's my reason. I'll change it because there is something I could agree with - backwards compatibility and not breaking it for whoever is already using it like that...

[Throne3d]

Sure it makes little sense, but the whole code around sending those messages made very little sense, hence I just placed it at the first place where i could, without changing too much.

The names of the methods correspond to what they do, on master, whereas your change causes a method with a weird name to be used for both. I don't think it's particularly controversial to suggest you change it?

lol

I'm not the one who set up the linting, and I don't feel I have 'sufficient authority' to suggest it change, but the current linting wants you to reduce the line length and if you don't it'll break Travis builds. This prevents us from checking, online, whether your code breaks tests.

And here we will disagree. IRC (any good IRC clien?) has /me formatting with a star, followed by "name did something" without what you would insert, the  something

My IRC client displays it as:

<Throne> | Blah
       * | Throne did something

(The star represents that it's not a regular message sent by a user, as I receive similar commands when I join the server notifying me of, e.g., the message of the day.)

Discord has /me actions display as _${text}_, which is why I suggest leaving it as is. If you enable compact mode on Discord you can see this – it displays like this, and this is how the bot currently does it. Your changes modify this to display (as in your screenshot) in italics with added star symbols (at the start and at the end, which doesn't seem to fit with what you said any good IRC client displays it as).

If you want to change it, I'd suggest changing it to **${author}** ${text} or **${author}** _${text}_ or something like that, to fit with how Discord displays its own /me commands, but I don't think this is necessary because (1) it will require tests to be changed so they pass, and (2) it requires users who prefer the old style to manually re-enable it. Instead, we could simply maintain the default and add the new functionality to configure it this way manually; it could even get a mention in the README as an alternate display. This seems like a lot of hassle, in comparison to that?

[Throne3d]

In response to your edited message and the updated commit:

So I'd still prefer you alter the discordMe in the README to be something I consider more sensible (\* **{$author}** {$withMentions} would work, for example), but that's an aesthetic preference as opposed to a functional one, so it's not necessary to get this merged.

This currently still breaks tests, despite the change, because the test for "Bot Events should send actions to discord" is checking the parameters sendToDiscord is called with – if you could append to the parameters the , true (for isAction) and alter the message to not include the underscores, as well as (like I briefly mentioned before) add a test that (1) the bot defaults to the formatting it did before, and (2) the bot uses the custom formatting if given, that'd be great.

[RheaAyase]

@Throne3d I've commited the changes, but I'm afraid that I don't really understand what's going on in the tests file...

[Throne3d]

That's okay! Thanks for the contribution; I'll get to modifying the tests myself.

[RheaAyase]

@Throne3d Either I'm looking into wrong file or I don't know why I can't find what you're mentioning... >_>

[RheaAyase]

Okay found it :D

[Throne3d]

You forgot to remove the underscores from formattedText :P

I guess I'll add the test for the custom formatting myself? Or are you planning on getting to that after?

[coveralls]

Coverage increased (+0.006%) to 99.259% when pulling 1cc0c7a on RheaAyase:master into ddd9c54 on reactiflux:master.

[RheaAyase]

I guess I'll add the test for the custom formatting myself? Or are you planning on getting to that after?

I'm not entirely sure what does it entail... Sure we use the thing to format it and stuff, but looking at the rest of the tests, they don't seem to handle custom formatting for standard messages either(?) https://github.com/reactiflux/discord-irc/blob/master/test/bot.test.js#L86

[Throne3d]

Sure they do, but later on:

• Could you also add a test, approximately here (to bot.test.js near the other formatting tests), to make sure the bot obeys the custom formatting when given?

(The link in the above quote from earlier points to: https://github.com/RheaAyase/discord-irc/blob/d0ebb9ed5e7631350b02dfbe1d96389d2586628f/test/bot.test.js#L665. A test should be added here for the custom formatting of an action, when sendActionToDiscord is called and the bot has a custom discordMe.)

It doesn't have a dedicated test that sendActionToDiscord produces the appropriate result, though, because previously they used the same mechanism – if you could add a test for this just after the one for sendToDiscord, back here, that would be good.

(Two tests in total – one near the beginning to ensure sendActionToDiscord performs the appropriate action, and one later on with all the other formatting ones to ensure it respects custom formatting.)

[ekmartin]

Closing this due to inactivity.

[RheaAyase]

@ekmartin my PR is complete. Tests do not have to be a part of a feature. Create an issue for the missing tests, accept the feature would be the correct course of action. Also don't expect busy professionals to create stuff for you as you whistle. I have a queue of a few months to get around to things.

[Throne3d]

I find myself confused. @RheaAyase, your message comes across as quite… annoyed? If the project only wants to accept new features that have tests associated (as otherwise, it might increase the effort to maintain the feature in the future), isn't that a decision for the project to make?

(I would also point out that you don't seem to have provided tests, and you seem to expect the changes to be merged without them and then for us … potentially busy professionals … to create tests, for you, to make sure it works…?)

[RheaAyase]

@Throne3d Yes annoyed as just closing my work off and throwing it to trash is quite rude. I don't seem to have provided tests but I have said that I will, when I have the time to - not before November.

Simply put, it's rude to close perfectly good PR without any discussion.

[Throne3d]

@RheaAyase I'm sure we'd be happy to reopen if someone provides tests? I wouldn't have considered this 'without any discussion', though – tests were requested and then there was inactivity and so it was closed. Closing the PR isn't an irreversible process, it just – to me – suggests it's not ongoingly worked on.

[RheaAyase]

Did you ask if there is nobody working on it? As I said, I'm busy, that doesn't mean that I dropped it...

[Throne3d]

I feel like you misread my point ("ongoingly" worked on) and I'm not sure I'm going to get anywhere with this.

[RheaAyase]

I see an open PR as an incentive and a reminder to work on it. Now that it's closed, well, it got dumped in priority for me.

[Throne3d]

I don't really think you added that comment in an attempt to be helpful. I think you are attempting to make me feel guilty or sad about the fact you are less likely to contribute.

I would prefer you not do that.

If you want to add code to this, feel free to, at some point that it's convenient to you. If you do not, then someone else may pick it up; I, in fact, may pick it up, at some time that's less inconvenient for me.

You did not respond to the pull request in over a month and a half, after having feedback, and nobody else stepped in to take over the issue. It may have been a bit abrupt to just close it, but it seems this repository has a standard of not accepting new features without tests – which seems reasonable – and of closing issues without enough information and pull requests that are not in a mergeable state after long enough without activity – which, again, seems reasonable.

I will see if I can get around to adding tests for this, sometime, because it seems like it may be useful to add. You are also free to add them at your own leisure and prompt me into reopening the issue.

Thank you for your attempt at a contribution, nonetheless.

[RheaAyase]

Please don't read between the lines. I am very open and straight forward person, had I meant that, I would have said that. I did not. I literally had this issue open between other 50 things in my browser for two months. I mean what I said, to the letter. There is no hidden agenda.

I simply think that the communication is very important in just about anything, and acting without asking is just bad practice, whether it's PRs, issues, or community moderation. I'm genuinely trying to improve things, not make them worse, and I am sorry if it came across that bad.

[Throne3d]

This seems like a fair point to raise. Perhaps it would be a good idea to mention on the issue that it's being considered to be closed for inactivity – @ekmartin? (I think I consider the "needs more info" tag I placed on several issues to be sufficient, but that or an equivalent wasn't placed here.)

It seemed implausible you were commenting on the issue that you would be less motivated to complete the PR solely to bring it to our attention so we would work together to make you completing it a more likely reality, which is why I assumed there was a hidden agenda – mostly the intentions behind it, making it seem less charitable. I suppose that was a heuristic misfiring, so I'm sorry also. I do genuinely mean that the feature would be a useful addition, and that I may add the code for tests myself, but that it may take a week or several for me to get around to it (and was, though probably is no longer, made less likely by the previous conversation), and in addition I was trying to honestly communicate my perspective on the situation myself.

[RheaAyase]

I'll get back to it, eventually, if you're not faster... It's not that much to not be possible, but right now I don't have even that half an hour or however long it would take...