Skip to content

Add Message.respond() convenience to nats-core#953

Draft
caspervonb wants to merge 3 commits into
mainfrom
nats-core-message-respond
Draft

Add Message.respond() convenience to nats-core#953
caspervonb wants to merge 3 commits into
mainfrom
nats-core-message-respond

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

Every reply currently requires await client.publish(msg.reply, data) plus a manual msg.reply is not None guard. Message.respond() mirrors nats.go's msg.Respond() and removes the boilerplate from the most common request/reply path.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 27, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 2m 15s —— View job


Review

  • Gather context and read PR diff
  • Read relevant source files
  • Analyze for correctness, type safety, asyncio patterns, and API conventions
  • Post inline comments for specific issues
  • Post overall review summary

The implementation is correct and the approach mirrors nats.go cleanly. All four Message construction paths (_handle_msg, _handle_hmsg for both futures and subscriptions) correctly wire up _client=self. The asyncio pattern is sound — respond() is a thin await client.publish() delegate with no new concurrency concerns.

One real bug: NoReplySubjectError is exported from nats.client.errors.__all__ but is never imported in nats-core/src/nats/client/__init__.py and is absent from nats.client.__all__. Every other error in this package (NoRespondersError, MaxPayloadError, SecureConnectionRequiredError, StatusError) is accessible as from nats.client import …. Callers catching NoReplySubjectError will be forced to reach into the internal nats.client.errors module — the test already demonstrates this implicitly by doing from nats.client.errors import NoReplySubjectError rather than from nats.client import NoReplySubjectError.

Two smaller issues are noted in inline comments: the raise NoReplySubjectError (class, not instance) convention, and _client leaking into the dataclass constructor due to init=True (the default) on a slots=True dataclass.

Comment thread nats-core/src/nats/client/message.py Outdated
RuntimeError: The message was not dispatched by a connected client.
"""
if self.reply is None:
raise NoReplySubjectError

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Raising the class directly (raise NoReplySubjectError) is valid Python (it auto-instantiates with no args), but raising an instance is the conventional pattern and matches all other raise sites in this codebase.

Suggested change
raise NoReplySubjectError
raise NoReplySubjectError()

Comment thread nats-core/src/nats/client/message.py Outdated
reply: str | None = None
headers: Headers | None = None
status: Status | None = None
_client: Client | None = field(default=None, repr=False, compare=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

With @dataclass(slots=True), _client becomes part of the generated __init__ signature, leaking an internal implementation detail into the public constructor. Any user manually constructing a Message will see it in their IDE autocomplete and help() output.

Consider field(default=None, repr=False, compare=False, init=False) and set it after construction at the call sites:

message = Message(subject=subject, data=payload, reply=reply)
message._client = self

This keeps the public constructor clean while still populating the field (the slot still exists, so attribute assignment works fine).

With slots=True the field showed up in the generated __init__,
leaking an internal detail into autocomplete and help(); populate it
by assignment at the dispatch sites instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant