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

WIP: feature(parser middleware): Add support for processing raw IRC lines. #295

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jirwin
Copy link
Collaborator

@jirwin jirwin commented Jan 11, 2015

This introduces a change that allows users to process any incoming raw
IRC messages before they are passed onto the final message parser.

This adds a few concepts:

  • Client.enableParseMiddleware(function): Add a function to the end of
    the middleware chain.
  • Client.disableParseMiddleware(function: Removes a function from the
    middleware chain.
  • Client.parser: The function that is used to parsed an incoming raw
    IRC message. This is run after all middlewares have completed.
    Defaults to the message parser at ./lib/parse_message.js. Custom
    parsers must return an object that matches what the existing parser
    returns.

As an example, the stripColors feature has been moved to a parser middleware.

Also tests.

Also some various cleanup I came across while working on this.

* @return {String} An IRC message stripped of colors.
*/
exports.stripColors = function stripColors(line) {
return line.replace(/[\x02\x1f\x16\x0f]|\x03\d{0,2}(?:,\d{0,2})?/g, '');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a module that does this? I'd be surprised if not. If not, I would consider extracting it into one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like https://www.npmjs.com/package/irc-colors does everything irc colors. Probably worthwhile switching to that, but out of scope for this PR. I'll open an issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue @ #297

@ghost
Copy link

ghost commented Jan 12, 2015

A more general comment: could you please rebase this to one commit, off of current master with my recent pull req?

@hnry
Copy link

hnry commented Jan 12, 2015

Just my 2 cents, I don't think the 'enableParseMiddleware' and 'disableParseMiddleware' should be on the Client object. Because I'd rather the Client object not be overloaded with APIs and it can slightly encourages weird stuff to happen if a user uses those functions while running.

So I'd rather see Client._parseMiddlewares be passed in on creation: var client = new Client(server, nick, { middleware: [customMiddleware, customMiddleware2, IRC.parser] })

The Client should have no idea how to add or disable middleware personally, it doesn't belong in the 'Client'.

And for users who want the option of modifying the middleware chain at runtime, it's still possible. But the API doesn't help them do it. Since no many people will want it, and it's kind of dangerous if they don't understand what they're doing.

And about the Client object, some people may have multiple clients, so you have to consider how it'll look in the end to create multiple Clients and how light these Client objects are.

@hnry
Copy link

hnry commented Jan 12, 2015

Oh and the plus side of making the middleware chain be passed in on Client creation is that you can always assume no passed in argument equals use the default node-irc middlewares for parsing. Which keeps the overall API simpler, otherwise with the enableParseMiddleware way it gets trickier to figure out if the user wants node-irc to just use the default middlewares, because you never know when they'll call it.

@hnry
Copy link

hnry commented Jan 12, 2015

And also can we have the default middleware chain be [stripColors, parseMessage, higherLevelEmitters], higherLevelEmitters being the .on 'join' , 'message', 'part', etc.

It's there by default to maintain the API most people are use to now, but we should have the option to remove it for people who don't want that.

@jirwin
Copy link
Collaborator Author

jirwin commented Jan 12, 2015

Thanks for the feedback. This is just the first step into making things a little less monolithic and providing a little more control to the user. I'll continue working on this.

@jirwin jirwin changed the title feature(parser middleware): Add support for processing raw IRC lines. WIP: feature(parser middleware): Add support for processing raw IRC lines. Jan 12, 2015
@ghost
Copy link

ghost commented Jan 12, 2015

I think this looks good so far! I want to test it out in real world usage. If it's still a WIP, are there other things you're planning on adding? Should I merge this as-is after testing it for a little bit, or wait?

@jirwin
Copy link
Collaborator Author

jirwin commented Jan 12, 2015

I'd like to take @LoveBear's feedback into consideration, as a first step. I think that defining the parser middleware being defined at the time of Client instantiation is a good idea, but want to make sure we get the interface right.

Don't merge this for now. Hopefully I'll be able to push changes later this evening.

This introduces a change that allows users to process any incoming raw
IRC messages before they are passed onto the final message parser.

This adds a few concepts:
* `Client.enableParseMiddleware(function)`: Add a function to the end of
  the middleware chain.
* `Client.disableParseMiddleware(function`: Removes a function from the
   middleware chain.
* `Client.parser`: The function that is used to parsed an incoming raw
  IRC message. This is run after all middlewares have completed.
  Defaults to the message parser at `./lib/parse_message.js`. Custome
  parsers must return an object that matches what the existing parser
  returns.

As an example, the stripColors feature has been moved to a parser
middleware.

Also tests.

Make Client.parseMiddlewares private.

Also call middlwares in the context of Client.

Add docs for parser middlewares.

Middleware should be passed as part of Client instantiation.
@jirwin jirwin force-pushed the jirwin/parseMessage-middlewares branch from 416e3d3 to 5ab451e Compare January 13, 2015 18:14
@jirwin
Copy link
Collaborator Author

jirwin commented Jan 13, 2015

@LoveBear I incorporated your feedback.

I still need to refactor the rest of the tests to use fixtures, and this should be ready to go.

@jirwin jirwin self-assigned this Apr 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants