ryanb / private_pub

Handle pub/sub messaging through private channels in Rails using Faye.
MIT License
864 stars 228 forks source link

exception handling and channel parsing #13

Closed zubin closed 13 years ago

zubin commented 13 years ago

Hi Ryan

I've added the following, with tests:

Apologies for not branching (this is my first pull request - I'll remember next time!)

Zubin

zubin commented 13 years ago

Turns out the first commit's exception handling wasn't as useful as I'd hoped, since rails still raises an ActionView::Template::Error (I tried moving the rescue to the helper too). I'll leave it there in case you know of a way around this.

ryanb commented 13 years ago

Thanks for submitting these ideas, however they are each separate issues and should be separate pull requests. Comments on these are below.

custom exception when server connection refused (otherwise it's a generic TemplateError)

You're right that there should be better handling when there's no connection, but I don't think raising a different exception is the best way. If you have ideas please comment in issue #10.

helpers now accept passing in objects, so you can do suscribe_to(@page)

I don't really understand what this would be doing. Is this creating a channel based on the object's name and id? Cool idea but I'm not certain if this is necessary or if it will end up cleaning up the code or not.

basic channel validation (initially, I didn't realise they had to start with a /)

I didn't think Faye was that strict on the format of channels. PrivatePub doesn't assume anything about the channel format and just passes it on to Faye so whatever it expects is what you'd have to use.

zubin commented 13 years ago

I'm closing this request to create a new one.

zubin commented 13 years ago

Okay, I'll create separate requests in future.

re: exception handling I've gone with failing silently for now, so at least other javascript gets executed.

re: passing objects into channel My app has live commenting on various resources, so each needed a separate channel. You're right - it's not entirely necessary, but after adding publish_to and subscribe_to several times, I wanted a way to standardise the channel names.

re: validation When I entered a channel name without a forward slash, it didn't work. Took a little time to figure out, so I thought adding this would be useful for others.

If you have any feedback, maybe we should switch to the other (open) pull request.

ryanb commented 13 years ago

My app has live commenting on various resources, so each needed a separate channel. You're right - it's not entirely necessary, but after adding publish_to and subscribe_to several times, I wanted a way to standardise the channel names.

It is a good idea, but I'm not certain it should be there or not. I will need to use this more and see if it feels like it improves the code or hides too much.

When I entered a channel name without a forward slash, it didn't work. Took a little time to figure out, so I thought adding this would be useful for others.

That's interesting, didn't know that, thanks. I've added it to issue #10 and I'm trying to gather common problems one runs into there. At the very least it should be added to the log when it encounters this problem, so if something ever isn't working I would tell them to look at the log.