Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TransformConfig into a trait #1064

Merged
merged 4 commits into from
Mar 27, 2023
Merged

Make TransformConfig into a trait #1064

merged 4 commits into from
Mar 27, 2023

Conversation

rukai
Copy link
Member

@rukai rukai commented Mar 7, 2023

To achieve this, we sprinkle in some dtolnay magic crates:

  • typetag - we will always need this to magically hook up our traits to serde
  • async_trait - the rust team seem pretty intent on landing some form of async traits one day so we should eventually be able to remove this

And finally we need a custom deserialize implementation for our chain config.
The problem was that typetagged structs with no fields would not deserialize from a yaml string.
e.g.
this would fail to deserialize:

chain_config:
  redis_chain:
    - NullSink

but this would work fine:

chain_config:
  redis_chain:
    - NullSink: {}

I do not really understand what the vec_transform_config custom deserializer is doing, but it clearly does fix the issue, allowing us to write our config as NullSink instead of NullSink: {}.
I can do a thorough investigation to better understand it if needed.
The implementation was a suggestion from dtolnay: dtolnay/typetag#40 (comment)
This solution does add a bunch of complexity but it is at least hidden away in its own module and doesn't affect users of the API.

@rukai rukai force-pushed the config_trait branch 2 times, most recently from 90d2142 to 18b5bb4 Compare March 8, 2023 21:39
@rukai rukai force-pushed the config_trait branch 2 times, most recently from 347d153 to 59a2f5d Compare March 14, 2023 05:10
@shotover shotover deleted a comment from github-actions bot Mar 20, 2023
@rukai rukai marked this pull request as ready for review March 20, 2023 23:44
@rukai rukai enabled auto-merge (squash) March 27, 2023 07:14
@rukai rukai merged commit 8809e22 into shotover:main Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants