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 218 forks source link

Call #to_compact_hash on all objects responding to it #231

Closed smaximov closed 3 years ago

smaximov commented 3 years ago

The current implementation of Compactable#to_compact_hash checks if a value inherits from Types::Base, which unfortunately is not the case for Types::Chat. So when we call #to_compact_hash for any Types::Base object that recursively contains a Types::Chat object, all values except for the chat object are converted to hash.

Steps to reproduce

update = Telegram::Bot::Types::Update.new(message: { chat: { id: 1 }, text: '/start' })
puts update.to_compact_hash

Expected behavior

The whole update is recursively converted to hash:

{:message=>{:chat=>{:id=>1}, :text=>"/start", :entities=>[], :caption_entities=>[], :photo=>[], :new_chat_members=>[], :new_chat_photo=>[]}

Actual behavior

The following hash is printed (note that :chat is not converted to hash):

{:message=>{:chat=>#<Telegram::Bot::Types::Chat:0x000000000f74bd00 @id=1, @type=nil, @title=nil, @username=nil, @first_name=nil, @last_name=nil, @all_members_are_administrators=nil, @photo=nil, @description=nil, @invite_link=nil, @pinned_message=nil>, :text=>"/start", :entities=>[], :caption_entities=>[], :photo=>[], :new_chat_members=>[], :new_chat_photo=>[]}}

Alternative

Since including a module inserts it into the inheritance chain, we could also check if the value is a subclass of Types::Compactable:

if value.is_a?(Compactable)
  value.to_compact_hash
else
  value
end

But I settled on calling #respond_to? because it's considered more idiomatic.

ivanovaleksey commented 3 years ago

Nice observation @smaximov. I wonder why Chat is not a subclass of Base? @atipugin is it on purpose or just a mistake?

smaximov commented 3 years ago

I wonder why Chat is not a subclass of Base?

I believe that's because Chat includes Virtus.model(finalize: false) and Base includes Virtus.model (without finalize: false).

atipugin commented 3 years ago

@smaximov good point, thanks! Wonder how it worked for years ;)

@ivanovaleksey you say! It was your commit - https://github.com/atipugin/telegram-bot-ruby/commit/6d9264eb9bba3bc487b95408b0003655269e5f98 :)

smaximov commented 3 years ago

Maybe it's worth to investigate why Chat is different, fix it and make it a subclass of Base again? I'm not familiar with Virtus but I can look into it this weekend.

ivanovaleksey commented 3 years ago

@atipugin I don't remember why I extracted Comparable module and deleted inheritable for Chat class. Maybe there was some kind of a circular dependency.

atipugin commented 3 years ago

@ivanovaleksey Yes i think it's because Chat depends on Message and vice versa. finalize: false defers constant resolving until we explicitly call Virtus.finalize.