-
Notifications
You must be signed in to change notification settings - Fork 71
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
Modify configuration to have provider-specific structs #114
Conversation
Signed-off-by: Anton Antonov <Anton.Antonov@arm.com>
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.
Excellent changes! A few things I particularly like:
- very good and original idea to use a enum composed of struct
- removing the
Option
makes the code much more simple - I did not even know that you could ignore some fields when matching a enum of structs 💥
Just a small comment about the serde
attribute
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.
Looks good! 👍
Signed-off-by: Anton Antonov <Anton.Antonov@arm.com>
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.
Thanks!
Just for future reference: for dealing with review comments you can also amend your commit and force-push it again.
// For providers configs in parsec config.toml we use a format similar | ||
// to the one described in the Internally Tagged Enum representation | ||
// where "provider_type" is the tag field. For details see: | ||
// https://serde.rs/enum-representations.html |
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.
Great explanation, thank you!
Use Enums of structs with provider specific options instead of a common struct.
Checks for missing parameters are not required in get_provider any more because it's done while parsing the config file into enums.
This change covers #70 and #103