V2 rebased #220
V2 rebased #220
Conversation
There was a problem hiding this comment.
I remember you pointed out specific ruby version to be a requirement. Would it be useful to add it to gemspec as a requirement?
spec.required_ruby_version = '>= 3.0.0'
| @@ -7,7 +7,6 @@ module Consume | |||
| # of messages to be handled at once | |||
| module BatchConsumption | |||
| extend ActiveSupport::Concern | |||
| include Phobos::BatchHandler | |||
There was a problem hiding this comment.
There are quite a few references to Phobos on sig/defs.rbs file. I believe they needs to be removed too.
|
|
||
| expect(Deimos::Logging). | ||
| to receive(:log_info). | ||
| with(hash_including(payload_keys: ["1", "2"])) |
There was a problem hiding this comment.
it 'should log message identifiers'. The expectation does not seem to test the message_id part and removes it.
I think the title should change if you say that this is the correct way in v2.
| @@ -159,12 +158,6 @@ Metrics/AbcSize: | |||
| - 'lib/deimos/utils/schema_controller_mixin.rb' | |||
There was a problem hiding this comment.
inline_consumer and schema_controller_mixin can be removed as the files are removed in the PR.
- 'lib/deimos/utils/inline_consumer.rb'
- 'lib/deimos/utils/schema_controller_mixin.rb'
| # @param handler_class [Class] | ||
| # @return [String,nil] | ||
| def topic_for_consumer(handler_class) | ||
| Deimos.karafka_configs.each do |topic| |
There was a problem hiding this comment.
Why is this not karafka_configs.each like in the above method? They both should be referring to the same method, isn't it?
def karafka_config_for(topic: nil, producer: nil)
if topic
karafka_configs.find { |t| t.name == topic}
elsif producer
karafka_configs.find { |t| t.producer_class == producer}
end
end
Work in progress for 2.0 version of Deimos backed by Karafka. For more info, see #219 .