search

ironcamel / vimchat

XMPP Chat for vim
84 stars 11 forks source link

Pr/presence notification #5

Closed philsmd closed 13 years ago

philsmd commented 13 years ago

PyNotification when a buddy changes online status. It is customisable which states will trigger a notification.

ironcamel commented 13 years ago

I just manually tested this feature and it works great. Thanks! Please commit the change I suggested above to this branch.

philsmd commented 13 years ago

Done. Thank you very much. There is other stuff I created a branch for but it is better to integrate/test these features first, when you are done w/ this 3 pull request I will start to push the other feature if you want. There is for example also some nice proxy mechanism that I (had to) implement (personal reasons). Best, Philipp

throughnothing commented 13 years ago

Thanks a bunch for the code philsmd! I'd really like to get a lot of your changes in. It does look like you've made all of your pull requests depend on each other, so in the future, if you could branch your feature/pr branches off of our master here, rather than your own master if you have other changes in there. I'm excited to get more of your code and pr branches in! Thanks again.

ironcamel commented 13 years ago

@philsmd, the MultiDict class you created doesn't make sense to me. It seems to set a key's value to a dict (default) if a key doesn't exist. Why does it do that?

philsmd commented 13 years ago

@ironcamel MultiDict is something I have used several times in python to create for example dictionaries of dictionaries (although other implementations are possible) . I used it also in this vimchat pull once, even if you probably could find several other ways to implement this. I think the code and the result is okay and it is like it is for example used here: http://parand.com/say/index.php/2007/07/13/simple-multi-dimensional-dictionaries-in-python/.

throughnothing commented 13 years ago

@philsmd it looks like we could simply use this:

http://docs.python.org/release/2.7/library/collections.html#collections.defaultdict

instead of needing a new class in our code. I'd go ahead and change it to use that rather than MultiDict.

throughnothing commented 13 years ago

@philsmd also, don't worry about the 80 line width for these pull requests, we can clean those up later. I didn't realize how many lines we already have that go over, so i'll do a cleanup branch after we get these in.

ironcamel commented 13 years ago

Thanks @philsmd!

ironcamel commented 13 years ago

One more thing, please remember to create future branches off of my master branch. Thanks.