-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
Enable settings conversion to CAF #1055
Conversation
5b2df1b
to
d46a232
Compare
d46a232
to
5f7ba01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly fine by me except for one small, subtle issue in the code.
Please change the test record for the unit test such that it contains a list that contains at least one different type, e.g., record.
I did not test this locally; the unit tests running through cover this functionality quite well.
f517e5b
to
0a1b3ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as is, just one remark/question: Why did you move from caf::settings
to caf::dictionary<caf::config_value>
? The former is an alias of the latter, but I find caf::settings
to be the more idiomatic type to enable conversion to.
The reason is that |
No description provided.