Refactored SerenityConfig #157
Conversation
….cpp Signed-off-by: bplotka <[email protected]>
Signed-off-by: bplotka <[email protected]>
| Option<double_t> totalAgentCpus = totalAgentResources.cpus(); | ||
|
|
||
| if (totalAgentCpus.isNone()) { | ||
| return Error(std::string(NAME) + " No total cpus in ResourceUsage"); | ||
| SERENITY_LOG(ERROR) << std::string(NAME) | ||
| << " No total cpus in ResourceUsage"; |
|
How about making SerenityConfig immutable? Make all set methods protected and make getSection method to return Option. |
Signed-off-by: bplotka <[email protected]>
|
That will make SerenityConfig incredible hard to test.. ): |
Signed-off-by: bplotka <[email protected]>
Signed-off-by: bplotka <[email protected]>
|
No, for test purposes we would just need to inherit a FakeSerenityConfig class (and test trough this) or be-friend the testing class (might be even better). |
| if (!inExec.has_executor_info()) { | ||
| SERENITY_LOG(ERROR) << "Executor <unknown>" | ||
| << " does not include executor_info"; | ||
| // Filter out these executors. |
There was a problem hiding this comment.
This comment no longer apply here. And bellow.
Comments lie all the times.
| * Variant type for storing multiple types of data in configuration. | ||
| */ | ||
| * Variant type for storing multiple types of data in configuration. | ||
| */ | ||
| using CfgVariant = boost::variant< |
There was a problem hiding this comment.
How about Value?
Storing 'variant' in name suggest internal structure that backs the field. When we will change the Variant class to something else, do we also want to change name there?
|
Nice work with putting docstrings in your APIs. One day we could enable #80 ;) |
Signed-off-by: bplotka <[email protected]>
Signed-off-by: bplotka <[email protected]>
No description provided.