search

mrshu / brutal-plugins

A set of plugins for brutal, the mighty chatbot
Apache License 2.0
4 stars 2 forks source link

sn: Added sender's nick to note's record. #65

Closed JakubNvk closed 9 years ago

JakubNvk commented 9 years ago

To: @mrshu

mrshu commented 9 years ago

A few things:

  1. This is not what we want. I doubt you've tested this change because if you did you would see that it just does not work (you somehow didn't update the send_notes function which is critical in order to keep sticky notes in working state).
  2. What this currently does is that it groups notes by users (that's not entirely true but I guess that's the idea here). While this is a way to go, I'd very much prefer to have metadata along with the text of the note since we might in the future add some more metadata to it (such as time of creation).
  3. If I read the code correctly then if person A adds some sticky notes and person B (which is not referenced in self.notes[to_user] yet) adds some other sticky note then sticky notes of person A will get lost. I doubt this is what you want.

In general, I'm glad you tried to fight with this but as I said, I'd appreciate if you could either create a Note object which could have an attribute from_nick or if you could use a dictionary for the same purpose (something like {'from_user': 'johndoe", 'note': 'Finally I caught you!'}). These would then get appended to self.notes[to_nick] in the same way as they are currently.

Do let me know if that makes more sense now or if I ommited some important detail.

Thanks!

mrshu commented 9 years ago

@JakubNvk thanks, this looks good to me.

Just squash it and we can merge it.

Adman commented 9 years ago

@mrshu @JakubNvk use simple quotes ' ' in strings where possible

JakubNvk commented 9 years ago

@Adman I really prefer " " for strings. It wasn't a problem in karma.py.

JakubNvk commented 9 years ago

Closing this and creating new PR since there are some problems with squashing.

mrshu commented 9 years ago

@Adman I do not think we need to be that restrictive. There is no real difference between the two.

I personally like to use double quotes on strings that will be interpolated or will contain the single quote symbol somewhere. However, I do not think we need to make that a rule (cc @pepol)