-
Notifications
You must be signed in to change notification settings - Fork 7
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): Config with multiple values #1258
Conversation
File comparision analysis report🔖 Info
Click to show memory sections
📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections
📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
Codecov Report
@@ Coverage Diff @@
## develop #1258 +/- ##
===========================================
- Coverage 96.20% 96.20% -0.01%
===========================================
Files 148 148
Lines 3719 3716 -3
===========================================
- Hits 3578 3575 -3
Misses 141 141
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
1824d2a
to
79ac2fa
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.
Nice work
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 for the PR, very interesting new feature 👍
that being said, as I wrote in the comments, I think it would be better to refactor Config instead of adding a new different type
a72cffd
to
d932774
Compare
d932774
to
c89dd0e
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.
great job merging both! 👍
a few suggestions to make the code even easier to read :)
libs/ConfigKit/include/ConfigKit.h
Outdated
template <std::size_t N> | ||
[[nodiscard]] auto read(Config<N> const &config) const -> std::array<uint8_t, N> | ||
{ | ||
if (FileManagerKit::File file {config.path(), "r"}; file.is_open()) { | ||
auto data = std::array<uint8_t, N> {}; | ||
file.read(data); | ||
|
||
return data; | ||
} | ||
|
||
return config.default_value(); | ||
} |
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.
it's actually possible to have a function with different return types provided it's used in constexpr
context
see https://gcc.godbolt.org/z/ren7Pa685
not sure if that would work there though, maybe for future improvements
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.
I tried something for read
3576dee
to
943b26b
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.
LGTM 👍 great work!
943b26b
to
1b47896
Compare
1b47896
to
119ab50
Compare
Version comparison
|
Kudos, SonarCloud Quality Gate passed! |
Validation