refactor: Clio Config #1544

PeterChen13579 · 2024-07-15T17:23:02Z

Implementation of new config definition + methods + UT

Steps that still need to be implemented:

Make ClioConfigDefinition and it's method to be as constexpr as possible
Getting User Config file and populating the values in ConfigDefinition while checking for constraints on user values
Replacing all the places where we fetch config values (by using config.valueOr/MaybeValue) to instead get it from Config Definition
Generate markdown file using Clio Config Description

codecov · 2024-07-15T18:18:06Z

Codecov Report

Attention: Patch coverage is 87.62376% with 25 lines in your changes missing coverage. Please review.

Project coverage is 69.56%. Comparing base (2a74a65) to head (fe84a9c).

Files	Patch %	Lines
src/util/newconfig/ConfigDefinition.cpp	86.66%	3 Missing and 3 partials ⚠️
src/util/newconfig/ObjectView.cpp	85.71%	1 Missing and 3 partials ⚠️
src/util/newconfig/ValueView.cpp	82.60%	0 Missing and 4 partials ⚠️
src/util/newconfig/ValueView.hpp	71.42%	2 Missing and 2 partials ⚠️
src/util/newconfig/ArrayView.hpp	85.00%	0 Missing and 3 partials ⚠️
src/util/newconfig/ConfigValue.hpp	91.17%	0 Missing and 3 partials ⚠️
src/util/newconfig/Array.hpp	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1544      +/-   ##
===========================================
+ Coverage    69.19%   69.56%   +0.37%     
===========================================
  Files          244      254      +10     
  Lines         9702     9904     +202     
  Branches      5384     5466      +82     
===========================================
+ Hits          6713     6890     +177     
- Misses        1589     1595       +6     
- Partials      1400     1419      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

godexsoft

Nice progress. Leaving some improvement ideas 👍

src/util/newconfig/Array.hpp

src/util/newconfig/ObjectView.cpp

tests/unit/util/newconfig/ValueViewTests.cpp

tests/unit/NewConfigTests.cpp

tests/unit/util/newconfig/ObjectViewTests.cpp

src/util/newconfig/ValueView.cpp

src/util/newconfig/ArrayView.cpp

src/util/newconfig/ArrayView.hpp

src/util/newconfig/ConfigConstraints.hpp

src/util/newconfig/ConfigDefinition.cpp

src/util/newconfig/ConfigValue.hpp

PeterChen13579 · 2024-07-17T03:01:31Z

Thank you for taking the time to review @godexsoft @cindyyan317 🙇

kuznetsss · 2024-07-22T12:45:47Z

There should be also tests for Array, ConfigConstraints (or delete them for now), ConfigDefinition, ConfigValue.

src/util/newconfig/ArrayView.cpp

src/util/newconfig/ArrayView.hpp

src/util/newconfig/ConfigDefinition.cpp

src/util/newconfig/ConfigDefinition.hpp

src/util/newconfig/ObjectView.cpp

src/util/newconfig/ValueView.hpp

tests/common/newconfig/FakeConfigData.hpp

tests/unit/NewConfigTests.cpp

tests/unit/util/newconfig/ArrayViewTests.cpp

src/util/UnsupportedType.hpp

src/util/newconfig/Array.hpp

tests/unit/util/newconfig/ArrayTests.cpp

src/util/newconfig/Array.hpp

src/util/newconfig/ArrayView.cpp

src/util/newconfig/ConfigDefinition.hpp

src/util/newconfig/ConfigValue.hpp

src/util/newconfig/ValueView.cpp

tests/unit/util/newconfig/ArrayViewTests.cpp

tests/unit/util/newconfig/ValueViewTests.cpp

src/util/newconfig/ArrayView.hpp

src/util/newconfig/ConfigDefinition.cpp

src/util/newconfig/ConfigDefinition.hpp

src/util/newconfig/ConfigDescription.hpp

src/util/newconfig/ObjectView.cpp

src/util/newconfig/ArrayView.hpp

tests/unit/NewConfigTests.cpp

src/util/newconfig/ArrayView.hpp

tests/unit/util/newconfig/ArrayViewTests.cpp

kuznetsss

👍

godexsoft

Overall this is a pretty cool system and i like the implementation 👍
This could be already be shipped but since this is a learning experience for you I'm leaving a few more suggestions and comments.

You can still use [[nodiscard]] more as well as const is missing in many places where it could be added.
It's also good to look for other algorithms than find_if - many times there will be some algo that's a better choice for the job at hand.

src/util/newconfig/Array.hpp

src/util/newconfig/ArrayView.hpp

src/util/newconfig/ConfigDefinition.cpp

src/util/newconfig/ObjectView.cpp

src/util/newconfig/ValueView.cpp

src/util/newconfig/ValueView.hpp

src/util/newconfig/ConfigDefinition.cpp

godexsoft

Changes since my last review look good 👍 🚀

Add constraint + parse json into Config Second part of refactoring Clio Config; First PR found [here](#1544) Steps that are left to implement: - Replacing all the places where we fetch config values (by using config.valueOr/MaybeValue) to instead get it from Config Definition - Generate markdown file using Clio Config Description

Fixes #1184 Previous PR's found [here](#1593) and [here](#1544)

PeterChen13579 added 4 commits July 15, 2024 12:59

implementation of new config defintion + methods + ut

cfbd215

Fix Doxygen

21092d0

add last doxygen

80bdbf3

fix minor assert error

d000f58

fix minor codecov

ce51e85

PeterChen13579 requested a review from godexsoft July 15, 2024 19:07

godexsoft requested changes Jul 16, 2024

View reviewed changes