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

Config refactoring #1500

Merged
merged 3 commits into from Feb 17, 2022
Merged

Config refactoring #1500

merged 3 commits into from Feb 17, 2022

Conversation

JohanMabille
Copy link
Member

The main changes are the followings:

  • use std::optional to store the CLI values. This allows to get rid of sentinel values to check if an option was defined..
  • merged the template classes Configurable and Wrapper into ConfigurableImpl.
  • renamed the WrapperBase class into ConfigurableImplBase. This class holds all the data members that do not depends on the template parameter.
  • renamed the ConfigurableInterface class into Configurable. This is now the unique interface to use for manipulating configurable objects.
  • Removed all the over-duplicated API. The only remaining methods are those implicitly depending on the template parameter but where we cannot deduce it (yaml parsing, json dumping, etc ...)
  • Fixed const-correctness issues.
  • Removed suboptimal copies of Configurable.

@JohanMabille JohanMabille force-pushed the config_refactoring branch 3 times, most recently from fd6fa0e to 600d2ca Compare February 16, 2022 21:50
@JohanMabille JohanMabille marked this pull request as ready for review February 16, 2022 22:59
@JohanMabille JohanMabille marked this pull request as draft February 17, 2022 11:23
@JohanMabille JohanMabille marked this pull request as ready for review February 17, 2022 20:06
@JohanMabille
Copy link
Member Author

JohanMabille commented Feb 17, 2022

@wolfv I think this is ready to go.

@wolfv wolfv merged commit 6568e2e into mamba-org:master Feb 17, 2022
@JohanMabille JohanMabille deleted the config_refactoring branch February 17, 2022 23:03
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.

None yet

2 participants