Skip to content

Conversation

@leshy
Copy link
Contributor

@leshy leshy commented Jan 29, 2026

addresses comments from #1114
Removes type: ignore[override] by changing subscribe signature to match the AllPubSub[Topic, Any] base class contract.

@greptile-apps
Copy link

greptile-apps bot commented Jan 29, 2026

Greptile Overview

Greptile Summary

This PR aligns LCMPubSubBase.subscribe() signature with its AllPubSub[Topic, Any] base class contract by removing the type: ignore[override] comment and updating the method signature.

Key changes:

  • Parameter type changed from topic: Topic | str to topic: Topic (matching base class)
  • Callback signature changed from Callable[[bytes, Topic | str], Any] to Callable[[bytes, Topic], None] (matching base class)
  • Removed unnecessary string handling logic since only Topic objects are now accepted
  • Simplified the pattern checking condition from isinstance(topic, Topic) and topic.is_pattern to just topic.is_pattern

The changes are safe because:

  • All existing usages in the codebase already pass Topic objects (verified in tests and transport.py)
  • The publish() method still accepts Topic | str for backwards compatibility where needed
  • Type safety is improved by matching the base class contract correctly

Confidence Score: 5/5

  • This PR is safe to merge with no issues - it's a type correctness fix
  • Score reflects a clean refactoring that improves type safety by aligning the implementation with the base class contract. All existing usages already pass Topic objects, making this a non-breaking change.
  • No files require special attention

Important Files Changed

Filename Overview
dimos/protocol/pubsub/impl/lcmpubsub.py Removed type: ignore[override] by aligning subscribe signature with AllPubSub[Topic, Any] base class contract

Sequence Diagram

sequenceDiagram
    participant Client
    participant LCMPubSubBase
    participant LCM
    participant Callback

    Client->>LCMPubSubBase: subscribe(topic: Topic, callback)
    
    alt topic.is_pattern
        LCMPubSubBase->>LCMPubSubBase: Create pattern handler
        LCMPubSubBase->>LCM: subscribe(pattern_str, handler)
        LCM-->>LCMPubSubBase: lcm_subscription
        Note over LCMPubSubBase: handler calls callback with<br/>Topic.from_channel_str(channel)
    else regular topic
        LCMPubSubBase->>LCM: subscribe(topic_str, lambda)
        LCM-->>LCMPubSubBase: lcm_subscription
        Note over LCMPubSubBase: lambda calls callback with<br/>original Topic object
    end
    
    LCMPubSubBase->>LCMPubSubBase: Set queue capacity to 10000
    LCMPubSubBase-->>Client: unsubscribe function
    
    Note over LCM,Callback: When message arrives
    LCM->>Callback: callback(msg: bytes, topic: Topic)

Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

leshy added 3 commits January 29, 2026 23:08
- LCMCommsConfig now uses LCMTopic with default_factory
- Add proper type params to PubSubComms and LCMSkillComms
- Fix SkillMsg type annotations with MsgType param
- Add explicit re-exports in protocol/service/__init__.py
- Wrap string topic in LCMTopic in pLCMTransport
- Add subscribe method declaration to mixin
- Fix _Subscription type to use PubSubBaseMixin
@leshy leshy changed the title fix: LCMPubSubBase.subscribe accepts Topic only fix: Pubsub typing Jan 29, 2026
@paul-nechifor paul-nechifor merged commit 4a7e8c4 into dev Jan 29, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants