[Fix] Remove deprecation warning #152
Conversation
… Client, TaskProcessor
… Client, TaskProcessor
antstorm
force-pushed
the
fix-config-deprecation
branch
from
195fa49 to
8d58b57
Compare
| @@ -130,7 +125,7 @@ class TestSerializer | |||
| event_type { Temporal::Api::Enums::V1::EventType::EVENT_TYPE_ACTIVITY_TASK_CANCELED } | |||
| activity_task_canceled_event_attributes do |attrs| | |||
| Temporal::Api::History::V1::ActivityTaskCanceledEventAttributes.new( | |||
| details: TestSerializer.to_details_payloads('ACTIVITY_ID_NOT_STARTED'), | |||
| details: Temporal::Configuration.new.converter.to_details_payloads('ACTIVITY_ID_NOT_STARTED'), | |||
There was a problem hiding this comment.
Shouldn't this be $converter.to_details_payloads(...)? Not sure why you are allocating a new one here.
There was a problem hiding this comment.
You're right, forgot to replace this one 👍
There was a problem hiding this comment.
Looks pretty good as a draft. Most of the changes were pretty repetitive so no comments on those. Main focus is probably on spec/unit/lib/temporal/converter_wrapper_spec.rb since that's where the ConverterWrapper gets tested. Looks complete to me.
I think the move to dependency injection is a good idea. I like the concept overall.
…configuration.converter
|
Is there any update here? @antstorm @chuckremes2 |
|
It would be awesome to merge this one 💯 |
|
Any updates on this? |
|
What is needed to get this over the line? It would be great to finally have a fix which removes the spam of log messages from |
|
Ping! Do y'all want help on this? |
|
@cdimitroulas @ukd1 @arrowcircle @santiagodoldan @0xTheProDev I've finally found some time and re-implemented this PR here — #314 |
Fixes #143
After #92 was merged we've added a deprecation warning to restrict references to
Temporal.configurationfrom the code. However we still rely on it heavily fromTemporal::Concerns::Payloadsthat is included in quite a few places. This PR removes this issue in favour of explicit dependency injection.The meat of this PR is the introduction of
Temporal::ConverterWrapper(that now houses all the convenience methods fromTemporal::Concerns::Payloads) and passing it explicitly where conversion functionality is needed.Ideally in the future we should limit conversion to the our API wrapper (
Connection::GRPC) and not allow any proto objects beyond that layer. But this is a much bigger refactor (many proto messages would require a local class).Tested using existing unit & integration specs. New unit specs added to cover added functionality.