search

atipugin / telegram-bot-ruby

Ruby wrapper for Telegram's Bot API
https://core.telegram.org/bots/api
Do What The F*ck You Want To Public License
1.35k stars 217 forks source link

Return defined types (Ruby objects) #284

Closed AlexWayfer closed 1 year ago

AlexWayfer commented 1 year ago

Hello.

I've noticed that all methods return response Hashes.

There is processing only for Types::Update: https://github.com/atipugin/telegram-bot-ruby/blob/0756675/lib/telegram/bot/client.rb#L36

I'd not change the current Api#call method, let's leave it with raw responses, but I'd change Api#method_missing to convert call['result'] into necessary types.

For example, you have BotCommand type: https://github.com/atipugin/telegram-bot-ruby/blob/0756675f5fbed794df30fb41a8081fc47462c723/lib/telegram/bot/types/bot_command.rb

But I don't see any possibility of its usage.

For such change, to not define every endpoint as a method, I'd convert Api::ENDPOINTS from Array to Hash, with corresponding type as a value. I hope Dry::Types will let me define Arrays of something.

Another detail about I'm not sure: should I check response['ok'] or response.status == 200 is enough? Can there be a response with status 200, but without result (with error)? I haven't found any info about this in the documentation.

And this would be a breaking, but I believe useful change, so we probably will be needed for a major release, like v2.0.

atipugin commented 1 year ago

Hey @AlexWayfer

Sounds interesting. Could you make some POC branch/PR for this? Just to play around and understand if it's convenient or not.

Another detail about I'm not sure: should I check response['ok'] or response.status == 200 is enough? Can there be a response with status 200, but without result (with error)? I haven't found any info about this in the documentation.

Docs don't mention anything about status code, so i'd not rely on it and keep using ok flag.

AlexWayfer commented 1 year ago

Sounds interesting. Could you make some POC branch/PR for this? Just to play around and understand if it's convenient or not.

Yeah, I think about it. I don't think it'd take a lot of change lines. I'd like to do this with specs, but right now I have issues with logging into test environment…

Docs don't mention anything about status code, so i'd not rely on it and keep using ok flag.

I wouldn't like to do so: any 4xx or 5xx error can have no ok field, even not JSON. We should have a double check there, or just rely on 200 — that's my quesiton.

AlexWayfer commented 1 year ago

Docs don't mention anything about status code, so i'd not rely on it and keep using ok flag.

I wouldn't like to do so: any 4xx or 5xx error can have no ok field, even not JSON. We should have a double check there, or just rely on 200 — that's my quesiton.

I've tested simple 400 errors — there is correct HTTP status, so I'd rely on it for now, and let's monitor. If we will catch cases with HTTP 200 and ok => false — it'd be very strange and we can investigate and fix it more proper.