Conversation
Aryex
changed the title
7472873 to
cb06201
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>
cb06201 to
7ba0a01
Compare
Signed-off-by: Alex Le <alex.le@improving.com>
| # rubocop:enable Lint/UselessMethodDefinition | ||
|
|
||
| def subscribe(*_channels, timeout_ms: 0) | ||
| raise CommandError, "subscribe is not supported in a pipeline or transaction" |
There was a problem hiding this comment.
Valkey::CommandError is the base for server-side command errors (WrongTypeError, NoScriptError, etc.). Calling subscribe on a pipeline is a caller mistake, and the existing precedent in this repo is ArgumentError: Pipeline#call with route: and ft_search/ft_aggregate inside a batch both raise ArgumentError with a "not supported inside pipelined/multi" message
| Metrics/ClassLength: | ||
| Exclude: | ||
| - 'lib/valkey.rb' | ||
| - 'lib/valkey/pipeline.rb' |
There was a problem hiding this comment.
Suggest removing this, and the Lint/UnusuedMethodArgument lint, and replacing the implementation in pipeline.rb with
%i[subscribe unsubscribe psubscribe ... pubsub_shardnumsub].each do |name|
define_method(name) do |*, **|
raise ArgumentError, "#{name} is not supported inside pipelined/multi"
end
end
Future unused-arg bugs in pipeline.rb ought to be caught.
| # valkey.try_get_pubsub_message | ||
| # # => nil | ||
| # | ||
| # @return [Valkey::Commands::PubSubCommands::Message, nil] the message, or `nil` when the queue is empty or the |
There was a problem hiding this comment.
Naming seems redundant, and pretty verbose.
Valkey::Commands::PubSubCommands::Message for what a user gets back form every get_pubsub_message is pretty long.
There was a problem hiding this comment.
Addressed in e3948eb.
Moved PushKind to internal class since users won't need to use it.
The other two types are now Valkey::Glide::PubSubState and Valkey::Glide::PubSubMessage.
|
Are you planning to beef up the testing around pubsub alongside pipelined, multi, and Pipeline.new? |
Signed-off-by: Alex Le <alex.le@improving.com>
Yes, pipeline pubsub support would be handled in a separate PR. |
Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
b0c2b69 to
e3948eb
Compare
2 participants
Summary
This PR refactor the
PubSubhandler class into a mixin module, per the comment here.Issue link
Related #135
Changes
PubSubhandler class removedpubsub_commands.rbLimitations
Testing
All integration tests passed locally. Unit tests assertions were not touched, only plumbing code.
Checklist
Before submitting the PR make sure the following are checked:
git commit -s) per the DCO.bundle exec rubocop) and pass.