refactor consumer option to validate the version #81
Conversation
add an abstract class to define the consumer options. Each Queue type can define the consumer options type: Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors consumer options by introducing an abstract ConsumerOptions class that provides a common interface for queue-specific consumer configurations. The refactoring generalizes the consumer interface to support different queue types while maintaining backward compatibility for stream queues.
- Adds an abstract
ConsumerOptionsbase class with validation and filter set methods - Refactors
StreamConsumerOptionsto inherit from the new abstract class and implement validation logic - Updates consumer creation interface to use the generalized
consumer_optionsparameter
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rabbitmq_amqp_python_client/entities.py | Adds abstract ConsumerOptions class and implements validation method for StreamConsumerOptions |
| rabbitmq_amqp_python_client/connection.py | Updates consumer creation to use generic consumer_options parameter and adds version validation |
| rabbitmq_amqp_python_client/consumer.py | Updates constructor parameter type to accept generic ConsumerOptions |
| rabbitmq_amqp_python_client/options.py | Updates filter class to use generic ConsumerOptions instead of specific stream options |
| tests/test_streams.py | Updates all test calls to use new consumer_options parameter and adds validation tests |
| tests/test_server_validation.py | Updates server version validation method calls to use new generic version checking |
| examples/* | Updates example code to use new consumer_options parameter |
| rabbitmq_amqp_python_client/init.py | Exports new ConsumerOptions class |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if application_properties is not None: | ||
| app_prop = application_properties.copy() |
There was a problem hiding this comment.
The variable app_prop is declared but never used in this method. The original code at line 340 that was removed likely used this variable. Either use app_prop in the method or remove the unnecessary assignment.
Copilot uses AI. Check for mistakes.
| "4.1.0", False | ||
| ): | ||
| raise ValidationCodeException( | ||
| "Stream filter by SQL requires RabbitMQ 4.1.0 or higher" |
There was a problem hiding this comment.
The error message is incorrect. Application properties filtering should mention 'application_properties' instead of 'SQL'. The error message should be 'Stream filter by application_properties requires RabbitMQ 4.1.0 or higher'.
| "Stream filter by SQL requires RabbitMQ 4.1.0 or higher" | |
| "Stream filter by application_properties requires RabbitMQ 4.1.0 or higher" |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Gsantomaggio
changed the title
This PR refactors consumer options by introducing an abstract ConsumerOptions class that provides a common interface for queue-specific consumer configurations. The refactoring generalizes the consumer interface to support different queue types while maintaining backward compatibility for stream queues.
Adds an abstract ConsumerOptions base class with validation and filter set methods
Refactors StreamConsumerOptions to inherit from the new abstract class and implement validation logic
Updates consumer creation interface to use the generalized consumer_options parameter