refactor: remove methoddispatch dependency and simplify _publish
#639
Conversation
WalkthroughThe pull request removes the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Channel
participant _publish
participant Handler
Caller->>Channel: publish_message(message)
Channel->>_publish: await _publish(message)
_publish->>_publish: isinstance(arg, Message)?
_publish->>Handler: Call publish_message logic
Handler-->>Caller: Return result
Caller->>Channel: publish_messages(messages)
Channel->>_publish: await _publish(messages)
_publish->>_publish: isinstance(arg, list)?
_publish->>Handler: Call publish_messages logic
Handler-->>Caller: Return result
Caller->>Channel: publish_name_data(name, data)
Channel->>_publish: await _publish(name, data)
_publish->>_publish: isinstance(arg, str)?
_publish->>Handler: Call publish_name_data logic
Handler-->>Caller: Return result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/639/features
ttypic
force-pushed
the
ECO-5638/get-rid-of-methoddispatch
branch
from
f2cd3d8 to
ede01a3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably/rest/channel.py (1)
104-106: Addparamsparameter topublish_name_datamethod.Line 104: The
publish_name_datamethod is missing theparamsparameter that exists inpublish_messageandpublish_messages. The publicpublish()dispatcher (line 126) passes**kwargstopublish_name_data, which means users calling.publish(name='x', data='y', params={...})will hit aTypeErrorat runtime since the method doesn't accept theparamskwarg.Update line 104 from:
async def publish_name_data(self, name, data, timeout=None):To:
async def publish_name_data(self, name, data, params=None, timeout=None):And update line 106 to pass
paramsalong withtimeout:return await self.publish_messages(messages, params=params, timeout=timeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
ably/rest/channel.py(2 hunks)pyproject.toml(0 hunks)
💤 Files with no reviewable changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
ably/rest/channel.py (1)
ably/types/message.py (1)
Message(24-231)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.11)
- GitHub Check: check (3.13)
- GitHub Check: check (3.10)
- GitHub Check: check (3.12)
- GitHub Check: check (3.8)
- GitHub Check: check (3.9)
- GitHub Check: check (3.7)
🔇 Additional comments (2)
ably/rest/channel.py (2)
21-21: LGTM: Dependency removed successfully.The removal of
SingleDispatchinheritance successfully eliminates themethoddispatchdependency while maintaining the same public API.
108-134: LGTM: Backwards compatibility preserved.The
publishmethod correctly maintains backwards compatibility for various calling patterns (kwargs with name/data, kwargs with messages, positional args) and properly delegates to the refactored_publishdispatcher.
| async def _publish(self, arg, *args, **kwargs): | ||
| if isinstance(arg, Message): | ||
| return await self.publish_message(arg, *args, **kwargs) | ||
| elif isinstance(arg, list): | ||
| return await self.publish_messages(arg, *args, **kwargs) | ||
| elif isinstance(arg, str): | ||
| return await self.publish_name_data(arg, *args, **kwargs) | ||
| else: | ||
| raise TypeError('Unexpected type %s' % type(arg)) |
There was a problem hiding this comment.
Validate list contents to prevent runtime errors.
The isinstance(arg, list) check accepts any list, not specifically lists of Message objects. Passing a list of non-Message objects (e.g., strings, dicts) will cause runtime errors downstream in __publish_request_body when attempting to call Message methods.
Consider adding validation:
async def _publish(self, arg, *args, **kwargs):
if isinstance(arg, Message):
return await self.publish_message(arg, *args, **kwargs)
elif isinstance(arg, list):
+ if arg and not all(isinstance(item, Message) for item in arg):
+ raise TypeError('All list items must be Message objects')
return await self.publish_messages(arg, *args, **kwargs)
elif isinstance(arg, str):
return await self.publish_name_data(arg, *args, **kwargs)
else:
- raise TypeError('Unexpected type %s' % type(arg))
+ raise TypeError(
+ f'Expected Message, list of Messages, or str for name, got {type(arg).__name__}'
+ )Additionally, consider adding type hints to prevent misuse:
async def _publish(
self,
arg: Message | list[Message] | str,
*args,
**kwargs
) -> None:🤖 Prompt for AI Agents
In ably/rest/channel.py around lines 78 to 86, the branch that accepts lists
doesn't validate that list elements are Message instances which can cause
runtime errors later; update the function signature to include type hints (e.g.,
arg: Message | list[Message] | str) and add explicit validation when arg is a
list: iterate the list, verify each item is an instance of Message, and raise a
TypeError with a clear message if any item is not; keep existing behavior for
Message and str branches and ensure the error message includes the offending
item's type/index for easier debugging.
Resolves #638
Getting rid of outdated and unmaintained
methoddispatchdependencySummary by CodeRabbit