search

hynek-urban / rocketchat-async

asyncio-based Python wrapper for the Rocket.Chat Realtime API.
MIT License
13 stars 9 forks source link

Add "raw" variants of some methods #12

Closed mschmitzer closed 2 months ago

mschmitzer commented 4 months ago

I'm trying to port an internal project from the discontinued rocketbot project to this library and ran into some missing information that is provided by the RocketChat API but discarded by the library. To provide this information without breaking API, this PR adds "raw" variants of the get_channels and subscribe_to_channel_messages methods.

To avoid code duplication, the existing implementations are rebased on the new "raw" variants.

hynek-urban commented 4 months ago

@mschmitzer Thank you! That is definitely a reasonable use case and I've been wondering about something similar myself. I'll either accept this PR or I'll do something along these lines - I'm currently thinking that for consistency, maybe all the (relevant) methods could accept an optional raw Boolean flag and the return value could be either transformed or raw based on this flag. What would you think about that?

mschmitzer commented 3 months ago

@hynek-urban I understand that having these _raw variants of existing methods isn't pretty, but turning this into a flag doesn't seem the right thing to do to me. I think callers would hard-code the flag value in most cases, so this would just make the implementation more "branchy". The subscribe_to_channel_messages case seems especially problematic because the signature of the callback would depend on the raw flag - just imagine writing the docstring for that :smile:

So I think the separate methods are the cleaner solution, even though it's not pretty. If you want to get this cleaned up eventually, you could deprecate the old variants and remove them in a major release.

hynek-urban commented 3 months ago

@mschmitzer I guess you're right. You're mentioning docstrings; I've been thinking about type hints and reached the same conclusion in the meantime :) Maybe it'll be consistent enough when all the relevant methods have "_raw" variants (where applicable).

hynek-urban commented 2 months ago

@mschmitzer It took me some time to get to it but let's do this :) I'll merge this PR and will add one or two more "raw" variants and update the README. Thanks for the contribution!.

mschmitzer commented 2 months ago

Great, thanks!