Add support for RPL_NAMREPLY and RPL_WHOREPLY

numberoverzero / bottom

asyncio-based rfc2812-compliant IRC Client

http://bottom-docs.readthedocs.io

MIT License

74 stars 23 forks source link

Add support for RPL_NAMREPLY and RPL_WHOREPLY #29

Closed doc-hex closed 8 years ago

doc-hex commented 8 years ago

Unpacking for WHO and NAMES responses.

coveralls commented 8 years ago

Coverage decreased (-1.8%) to 98.196% when pulling 6f76dfbd52ea86beaf1db7f9e97b779273e0321c on peter-conalgo:master into af0b11afe99a9af3a36bb39c4f7ececcadb8d0d2 on numberoverzero:master.

coveralls commented 8 years ago

Coverage remained the same at 100.0% when pulling f2cb8f937a2b3cbd118743ffc2213c8f2941ee8b on peter-conalgo:master into af0b11afe99a9af3a36bb39c4f7ececcadb8d0d2 on numberoverzero:master.

doc-hex commented 8 years ago

Ok, I think I'm done now! Whew. Rigorous setup you've got here.

numberoverzero commented 8 years ago

This is excellent! I agree that pack/unpack and associated tests should be a lot easier to extend; it's certainly the least maintained section of code.

Only question: I thought I pulled the names of reply strings from rfc2812; is there any reference to it being WHOREPLY instead of RPL_WHOREPLY?

I want to give it another review tomorrow but I'll probably end up merging as-is. If you don't mind, please add yourself to the Contributors section of the README. No pressure if you'd prefer not to.

coveralls commented 8 years ago

Coverage remained the same at 100.0% when pulling a7fa3a82ece52971aa649adb22cc35ae066f475e on peter-conalgo:master into af0b11afe99a9af3a36bb39c4f7ececcadb8d0d2 on numberoverzero:master.

doc-hex commented 8 years ago

Yes, RPL_WHOREPLY is the correct value from the RFC. I'm not familiar with IRC protocol at all, but from what I've learned making this change, it seems like only the numeric value is used at the wire-level (352) and I found the wording of this constant to be annoying (reply+reply).

It's visible to the users of the library when they register a trigger for this event, and I saw that another IRC library had called the same event WHOREPLY and that seems so much more natural, etc...

I'm not married to it, just give the word and I'll change it back. A longer-term solution would be a way to alias the event codes. Similar issues, IMHO, with RPL_NAMREPLY which is missing an E for some historical reason.

numberoverzero commented 8 years ago

Aliases are certainly something I want to support; I'll open an issue.

I'd like to keep RPL_WHOREPLY, even though it is ugly. Since most networks are using codes (well, at least freenode and a few python servers I could find) I would rather stick to the RFC for now.

If you don't mind that change, I'm happy to merge.

Thanks again for implementing this, even through the nitpicking!

coveralls commented 8 years ago

Coverage remained the same at 100.0% when pulling e269061c73d90e03ede79071a24b4e0d5c980f8b on peter-conalgo:master into af0b11afe99a9af3a36bb39c4f7ececcadb8d0d2 on numberoverzero:master.

numberoverzero commented 8 years ago

:rocket: