Switch to broker.serializer instead of JSON-only solution #14
Conversation
| # in `AsyncKicker._prepare_message`. | ||
| # The trick can be removed later after adding explicit `bytes` support. | ||
| if ( # noqa: WPS337 | ||
| isinstance(pipeline_data, str) |
There was a problem hiding this comment.
I'm wondering is this because of this line:
Maybe before merging we can add support for bytes in labels, by removing this str modifier? Since I really don't like how it looks. Guess removig str should give no effect, bu might break compatibility. I will find out how to make it possible without big compatibility problems.
There was a problem hiding this comment.
Yes, exactly. It is the result of str(label) conversion.
I suspect that the plain str(...) removal is not enough. Let me experiment first and come up with a PR for taskiq core project.
There was a problem hiding this comment.
Yes. I tried removing it and broke tests.
|
I also had no time to review it, sorry. Was busy as a bee on work. |
Don't rush please, a few days don't change the university :) |
The code works well but contains a dirty hack for parsing pipeline data from task labels.
Pipeline
loads()/dumps()areloadb()/dumpb()now.Step's
loads()/dumps()are gone away, seems like they don't override anything actually but all have the same code.I'm not sure if the PR should be merged as is.
@s3rius please consider it as the idea demonstration.
If you agree with the code in general, I'll make
bytessupport to message labels first, and remove the hack later.P.S. Sorry for two-week delay. Tests were hanging without any exception or log message. It took a while to figure out how to debug the problem.