fix: remove PubSub API - #217
Merged
Merged
Conversation
This was referenced Aug 1, 2026
Aryex
force-pushed
the
alexl/agent/pubsub-client-privatize
branch
from
August 4, 2026 20:56
136ab82 to
8bb7bd9
Compare
Aryex
force-pushed
the
alexl/agent/pubsub-client-privatize
branch
from
August 4, 2026 21:15
d710923 to
e82b715
Compare
Aryex
marked this pull request as ready for review
August 4, 2026 21:15
jamesx-improving
approved these changes
Aug 5, 2026
jamesx-improving
left a comment
jamesx-improving
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: Approve
0 high-confidence findings (3 doc nits).
Summary
Small, focused change that hides the unfinished Pub/Sub subscription surface from the 1.0.0 public API by marking subscribe / unsubscribe / psubscribe / punsubscribe / ssubscribe / sunsubscribe and the FFI push callback pubsub_callback as private, plus README/CHANGELOG updates.
Verification performed
- Visibility propagates through nested include. Confirmed via a standalone Ruby repro that
private :subscribeinsidePubSubCommandscarries throughCommands.include PubSubCommandsintoValkey.include CommandsandPipeline.include Commands. Callingclient.subscribe(...)raisesNoMethodError: private method 'subscribe' called for an instance of Valkey, matching the docstring claim. - FFI callback still wires up.
lib/valkey.rb:263passesmethod(:pubsub_callback)intoBindings.create_client_from_uri. Ruby'sMethodAPI and FFI callback dispatch both bypass visibility, so makingpubsub_callbackprivate does not break the incoming Pub/Sub push path. Verified with a standalone repro. - No internal callers. Grepped
lib/,test/valkey/,test/cluster/,test/lint/, andexamples/forsubscribe/unsubscribe/ etc. — no callers other than the definitions inpubsub_commands.rb. Privatization does not break any in-tree code paths. pubsub()dispatcher cannot re-expose the private methods. The convenience method at line 262 buildssend("pubsub_#{subcommand}", *args), sopubsub(:subscribe)would look uppubsub_subscribe, which does not exist and would raiseNoMethodError. It cannot accidentally reach the privatesubscribe.- Not covered by tests.
test/lint/pubsub_commands.rbonly exercisespublish/spublish/pubsub_*introspection commands (all of which remain public); no test covers the new "calling subscribe raises NoMethodError" contract. Consistent with the PR description ("n/a, API removal only") — not a defect, but worth noting.
Doc nits (optional)
README.md:325— Reads as past tense: "The Ruby client partially implemented the Pub/Sub GLIDE-Core API…". Likely intendedimplements.CHANGELOG.md:7–8— The bullet has a hard newline inside the sentence ("…to be added in\nlater releases."). Markdown renders as a space, but reflowing to one line reads better.lib/valkey/pubsub_callback.rb:5— Trailing whitespace:"# Pub/Sub API is not yet ready. Use at your own risk. "has a stray trailing space. RuboCop territory; not a defect.
LGTM.
currantw
reviewed
Aug 5, 2026
| Pub/Sub is currently not supported and is not ready for use. The Ruby client partially | ||
| implemented the Pub/Sub GLIDE-Core API and full support is planned for future release. | ||
|
|
||
| Use the current Pub/Sub API at your own risk. |
Collaborator
There was a problem hiding this comment.
Calling this raises NoMethodError
Is it even possible to "Use ... at your own risk"? Should we just say "don't use"?
Comment on lines
+5
to
+8
| ### Breaking Changes | ||
|
|
||
| * Ruby: Pub/Sub subscription commands are no longer public API. `subscribe`, `unsubscribe`, `psubscribe`, `punsubscribe`, `ssubscribe`, and `sunsubscribe` are now private, and the internal FFI push callback `Valkey::PubSubCallback#pubsub_callback` is private as well. Full pub/sub implementation to be added in | ||
| later releases. |
Collaborator
There was a problem hiding this comment.
Do we need Breaking Changes pre-GA release?
Collaborator
Author
There was a problem hiding this comment.
good point. To be removed.
currantw
reviewed
Aug 5, 2026
Comment on lines
3
to
5
| ## Pending | ||
|
|
||
| ### Changes |
Collaborator
There was a problem hiding this comment.
Nit. Was this intentional? If Pending is empty, maybe just remove?
xShinnRyuu
reviewed
Aug 6, 2026
xShinnRyuu
left a comment
xShinnRyuu
left a comment
Collaborator
There was a problem hiding this comment.
Mostly just nits comments so ill approve, but ideally you fix the inline comments
- Update description to reflect that entire PubSub surface is removed, not just subscription commands
- Consider adding regression test - Single test asserting NoMethodError when calling client.subscribe(...)
| module Commands | ||
| # This module contains commands related to Valkey Pub/Sub. | ||
| # Pub/Sub is not yet supported and is partially implemented. | ||
| # Use at your own risks. |
Collaborator
There was a problem hiding this comment.
typo Use at your own risks -> Use at your own risk
| # Publish a message to a channel. | ||
| # | ||
| # @api experimental | ||
| # @note EXPERIMENTAL: usable today, but part of the Pub/Sub surface and |
Collaborator
There was a problem hiding this comment.
Should probably update this usable today since its wont be usable with this PR
xShinnRyuu
approved these changes
Aug 6, 2026
Signed-off-by: Alex Le <alex.le@improving.com>
Aryex
force-pushed
the
alexl/agent/pubsub-client-privatize
branch
from
August 6, 2026 17:30
b916df3 to
6b9b83f
Compare
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Withholds the Pub/Sub subscription commands from the public API for release 1.0.0. Clearly document that Pub/Sub API is not ready for use.
Issue link
Related #216
Features / Changes
What changed:
subscribe,unsubscribe,psubscribe,punsubscribe,ssubscribe, andsunsubscribeare no longer callable onValkeyorValkey::Pipeline.Valkey::PubSubCallback#pubsub_callbackis private as well.Pub/Sub API is not documented as experimental and not ready for use.
Checklist
Before submitting the PR make sure the following are checked:
bundle exec rubocop) and pass.release-1.0