Add Content#empty? to fix Anthropic NoMethodError#734
Add Content#empty? to fix Anthropic NoMethodError#73455728 wants to merge 1 commit intocrmne:mainfrom
Conversation
Fixes crmne#729 RubyLLM::Content did not implement #empty?, causing NoMethodError in Anthropic provider code paths that check `msg.content.nil? || msg.content.empty?` when content is a Content instance (e.g. messages with ActiveStorage attachments). Content#empty? now returns true when text is blank (nil, empty, or whitespace-only) and there are no attachments, consistent with String#empty? and Array#empty? semantics.
|
I've opened a PR that aims to fix this on all providers and raising a meaningful error. |
|
Thanks for flagging this — I had a look at #733 and it’s a solid approach for the HTTP layer. There’s also the matter of the existing respond_to?(:empty?) guards scattered across providers (bedrock/media.rb, gemini/tools.rb, anthropic/tools.rb) — without Content#empty?, those silently treat every Content instance as non-empty, which isn’t quite what their authors intended. I’d be delighted if both could land — one hardens the transport, the other completes the domain model. But of course, happy to hear your thoughts. |
|
@55728 That's a broader decision: is an empty response an error or an acceptable case. My gut feeling tells me, we should have some content on the response, but I do not know every provider and that may pose an API change. Edit: there's another PR open on the same: #747 Flagging this to @crmne (sorry to flag you, but if we can have a decision here both PR's can be addressed and the issue fixed) |
|
Good call flagging @crmne — thanks for that. Agreed that the empty-response semantics are a broader design question. For what it’s worth, I believe this PR is independent of that decision: Content#empty? is about giving the domain object a method that Ruby containers conventionally have, and should be safe to land either way. Happy to wait for a steer on the bigger picture. |
What this does
Fixes #729
Summary
Adds
RubyLLM::Content#empty?somsg.content.nil? || msg.content.empty?no longer raisesNoMethodErrorwhenmsg.contentis aContentinstance.Why
@text.to_s.strip.empty? && @attachments.empty?Three shapes were considered:
def empty?; false; end, leaning on the constructor checkraise ... if @text.nil? && @attachments.empty?. That check only rejects aniltext, though —Content.new('')andContent.new(' ')both construct successfully, so a blanketfalsewould misrepresent those instances.anthropic/chat.rb:179andanthropic/tools.rb:19to use the defensiverespond_to?(:empty?) && empty?idiom already present inanthropic/tools.rb:49,bedrock/media.rb:20, andgemini/tools.rb:19. Workable, but leavesContentas the only nearby domain object without#empty?, which is a bit surprising for a container-like Ruby type.Contentan#empty?withString#empty?/Array#empty?semantics — blank when there is no visible text and no attachments. The method name alone predicts the behavior, and the rule holds even if the constructor invariant is relaxed later.Side effect (bonus)
bedrock/media.rb:20,gemini/tools.rb:19,gemini/tools.rb:50, andanthropic/tools.rb:49already use the defensivecontent.respond_to?(:empty?) && content.empty?idiom. Until nowContentdid not respond toempty?, so those guards silently treated everyContentinstance as non-empty. With#empty?defined, those call sites behave as their authors intended for blank content.Out of scope (happy to fold in if preferred)
anthropic/chat.rb:179andanthropic/tools.rb:19still use baremsg.content.empty?. Commit 4371a1b introduced the defensiverespond_to?(:empty?)pattern inanthropic/tools.rb:49; aligning these two lines with that convention would be a small follow-up — happy to handle it in a separate PR, or fold it into this one if that fits the repo style better.Type of change
Scope check
Quality check
overcommit --installand all hooks passspec/ruby_llm/content_spec.rbpasses (5 examples, 0 failures)models.json,aliases.json)AI-generated code
API changes
RubyLLM::Content#empty?)