ekmartin / slack-irc

Connects Slack and IRC channels by sending messages back and forth.
MIT License
588 stars 157 forks source link

Improved text formatting in both directions #51

Closed da-anda closed 8 years ago

da-anda commented 9 years ago

Currently, text formatting is not converted into the native formats of the recipient in both directions. This PR aims to add support for the basics, which is bold, underlined and italic. Colours are not yet supported.

It also improves the output of URLs and mails on IRC client side, which currently do not support the pipe syntax by Slack and thus urls like <http://google.com|google.com> end up as http://google.com|google.comin IRC instead of just http://google.com. Same for mails, where the mailto: prefix is also being removed.

I wasn't sure how to name the parsing method for Slack messages. Ideally the old parseText function would be renamed to parseTextForIRC or alike, but I didn't want to break backwards compat. Please tell me what you prefer here and I'll adjust the PR accordingly.

ekmartin commented 9 years ago

No worries about breaking backwards compatibility, as there's nothing that relies on slack-irc's functions to be an external API.

As for the pull request, this is definitely something that could be interesting. Which IRC clients makes use of these escape sequences for formatting though? If it's specific for only a few clients I'm not sure if it's that necessary, however if it's more of a standard it could be cool.

da-anda commented 9 years ago

I'm not sure if it's a defined standard (haven't found anything that looked like a official standard document), but to my knowledge most IRC client should support it - at least bold and underlined (italic probably not). All the various clients used by our team members do support bold and underlined, and colours ofc. Here is the reference I used: https://www.codeux.com/textual/help/Text-Formatting.kb

What should I do about the failing check (line length) - ignore, rewrite it or add a line break? I'm new to coding for node.js etc

As for the function names - so it's ok if I rename it? Any wishes for the method names?

ekmartin commented 9 years ago

Sorry for the late answer, seems like WeeChat at least uses somehow similar escape sequences to what you've used here. Feel free to rename the functions to something more suitable, like parseTextFromSlack and parseTextFromIRC. As for the long line, you can add a line break before function.

Would be awesome if you could write some test cases for the parsing changes too. Would be much easier to understand the regexes that way. Something like this: https://github.com/ekmartin/slack-irc/blob/master/test%2Fbot.test.js#L107 - use one test case for each, sort of like:

describe('IRC Text Parsing', function() {
  it('should convert bold text', function() {
    var text = 'message';
    this.bot.parseTextFromIRC('\u0002' + message).should.equal('*' + message + '*');
  });

  it('should convert italics', function() {
    var text = 'message';
    this.bot.parseTextFromIRC('\u001d' + message).should.equal('_' + message + '_');
  });
});

Just ask if you need any help with the tests, I'm online in slack-irc's Gitter channel here: https://gitter.im/ekmartin/slack-irc

da-anda commented 9 years ago

I'll add some test cases - might take a few days though

ekmartin commented 9 years ago

No problem :)

ekmartin commented 8 years ago

Any update regarding the tests? Feel free to shout out if you don't have time, I'd be glad to help.

da-anda commented 8 years ago

sorry, having a busy time atm and the little time for open source I have atm is needed on a different project. Feel free to take over if you want this merged in time

ekmartin commented 8 years ago

Closing this in the meantime.