mmyster/feature/config kit #598
Conversation
MMyster
commented
Mar 17, 2022
MMyster
commented
- ✨ (file_system_kit): Add 'ConfigKit'
- ✨ (config_kit): Add 'ConfigKit'
Codecov Report
@@ Coverage Diff @@
## develop #598 +/- ##
===========================================
+ Coverage 98.16% 98.18% +0.01%
===========================================
Files 92 94 +2
Lines 1966 1985 +19
===========================================
+ Hits 1930 1949 +19
Misses 36 36
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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
|
MMyster
force-pushed
the
mmyster/feature/config_kit
branch
5 times, most recently
from
23f0b4d
to
5913528
Compare
MMyster
force-pushed
the
mmyster/feature/config_kit
branch
from
5913528
to
156ebd3
Compare
There was a problem hiding this comment.
Merci @MMyster pour cette nouvelle implémentation! 👍
c'est beaucoup mieux que la première fois, beaucoup plus clair et beaucoup plus facile à utiliser.
j'ai fait pas mal de retours pour simplifier la lecture du code + quelques uns plus structuels.
N'hésite pas à poser des questions ici ou sur slack si ce n'est pas clair.
MMyster
force-pushed
the
mmyster/feature/config_kit
branch
11 times, most recently
from
dcfcdc0
to
1871cf4
Compare
There was a problem hiding this comment.
merci @MMyster pour les modifications! c'est vraiment très bien!
j'ai fait quelques remarques finales, j'aimerais que @YannLocatelli jette un coup d'oeil aussi.
et on sera bon pour merge! :)
libs/ConfigKit/include/ConfigList.h
Outdated
| namespace leka::ConfigList { | ||
|
|
||
| const Config bootloader_battery_level_hysteresis("bootloader_battery_level_hysteresis"); | ||
|
|
||
| } // namespace leka::ConfigList |
There was a problem hiding this comment.
| namespace leka::ConfigList { | |
| const Config bootloader_battery_level_hysteresis("bootloader_battery_level_hysteresis"); | |
| } // namespace leka::ConfigList | |
| namespace leka::config { | |
| namespace bootloader { | |
| inline constexpr auto battery_level_hysteresis = Config {"bootloader_battery_level_hysteresis"}; | |
| } // namespace bootloader | |
| } // namespace leka::config |
There was a problem hiding this comment.
avec le default dans le ctor
inline constexpr auto battery_level_hysteresis = Config {"bootloader_battery_level_hysteresis", 42};
libs/ConfigKit/source/Config.cpp
Outdated
| } | ||
| auto input = std::array<uint8_t, 1> {}; | ||
| _file.read(input); | ||
| _file.close(); |
There was a problem hiding this comment.
tu as besoin de close à chaque fois?
si une file est déjà is_open et que tu essayes d'open("new_file"), il se passe quoi?
je suis un peu gêné par le open/close qui est assez redondant.
j'ai presque envie d'avoir le file sur le stack et quand ça sort du scope, le destructor est détruit et ça ferme le fichier automatiquement.
qu'en penses-tu?
There was a problem hiding this comment.
Le fait d'instancier un objet FileManagerKit::File à chaque appel de fonction read/write ne risque pas d'être trop lourd?
Pour le point à propos de l'utilité du close, ça pourrait être intéressant de faire le test en écrivant sur 2 fichiers à la fois sans fermer l'un l'autre pour voir ce qu'il s'y passe.
Mais si on ne close jamais, est-ce raisonnable de garder les n fichiers ouverts pour les n configurations?
libs/ConfigKit/include/ConfigKit.h
Outdated
|
|
||
| struct Config { | ||
| public: | ||
| explicit Config(const std::filesystem::path &path) : _path(_default_parent_path / path) {} |
There was a problem hiding this comment.
il faut que tu prennes le _default_value dans le constructeur pour pouvoir avoir des différents en fonction de la config
libs/ConfigKit/CMakeLists.txt
Outdated
| @@ -0,0 +1,28 @@ | |||
| # Leka - LekaOS | |||
| # Copyright 2021 APF France handicap | |||
There was a problem hiding this comment.
| # Copyright 2021 APF France handicap | |
| # Copyright 2022 APF France handicap |
spikes/lk_config_kit/main.cpp
Outdated
| @@ -0,0 +1,70 @@ | |||
| // Leka - LekaOS | |||
| // Copyright 2020 APF France handicap | |||
There was a problem hiding this comment.
| // Copyright 2020 APF France handicap | |
| // Copyright 2022 APF France handicap |
libs/ConfigKit/source/Config.cpp
Outdated
| @@ -0,0 +1,38 @@ | |||
| // Leka - LekaOS | |||
| // Copyright 2021 APF France handicap | |||
There was a problem hiding this comment.
| // Copyright 2021 APF France handicap | |
| // Copyright 2022 APF France handicap |
spikes/lk_config_kit/main.cpp
Outdated
| log_info("A message from your board %s --> \"%s\" at %i s", MBED_CONF_APP_TARGET_NAME, hello.world, | ||
| int(t.count() / 1000)); | ||
|
|
||
| if (auto write = configkit.write(i++, ConfigList::bootloader_battery_level_hysteresis); !write) { |
There was a problem hiding this comment.
J'aurai tendance à donner d'abord l'argument du fichier avant de donner la data plutôt que l'inverse, c'est plus intuitif d'indiquer le contenant avant le contenu
| if (auto write = configkit.write(i++, ConfigList::bootloader_battery_level_hysteresis); !write) { | |
| if (auto write = configkit.write(ConfigList::bootloader_battery_level_hysteresis, i++); !write) { |
There was a problem hiding this comment.
je suis d'accord!
@MMyster il y a aussi un code smell pour i++
je pense que tu veux ++i et pas i++ et ensuite il faut faire l'assignment avant comme demande sonarcloud
libs/ConfigKit/source/Config.cpp
Outdated
| } | ||
| auto input = std::array<uint8_t, 1> {}; | ||
| _file.read(input); | ||
| _file.close(); |
There was a problem hiding this comment.
Le fait d'instancier un objet FileManagerKit::File à chaque appel de fonction read/write ne risque pas d'être trop lourd?
Pour le point à propos de l'utilité du close, ça pourrait être intéressant de faire le test en écrivant sur 2 fichiers à la fois sans fermer l'un l'autre pour voir ce qu'il s'y passe.
Mais si on ne close jamais, est-ce raisonnable de garder les n fichiers ouverts pour les n configurations?
libs/ConfigKit/source/Config.cpp
Outdated
| auto input = std::array<uint8_t, 1> {}; | ||
| _file.read(input); |
There was a problem hiding this comment.
Question bête peut-être, pourquoi avoir utilisé un std::array plutôt que directement un uint8_t?
| auto input = std::array<uint8_t, 1> {}; | |
| _file.read(input); | |
| auto input = uint8_t {0}; | |
| _file.read(input, 1); |
There was a problem hiding this comment.
La fonction prend un std::span en paramètre. Dans les tests, je n'utilisais que des pointeurs ou des std::array pour les fonctions read et write.
libs/ConfigKit/tests/Config_test.cpp
Outdated
| TEST_F(ConfigTest, initializationConfig) | ||
| { | ||
| Config config {config_path}; | ||
| ASSERT_NE(nullptr, &config); | ||
| const std::filesystem::path expected_path = "/tmp/test_config.conf"; | ||
| ASSERT_EQ(expected_path, config.path()); | ||
| } | ||
|
|
||
| TEST_F(ConfigTest, initializationWithDefaultParentPathConfig) | ||
| { | ||
| const std::filesystem::path new_config_path = "test_config.conf"; | ||
| Config config {new_config_path}; | ||
| ASSERT_NE(nullptr, &config); | ||
| const std::filesystem::path expected_path = "/fs/conf/test_config.conf"; | ||
| ASSERT_EQ(expected_path, config.path()); | ||
| } |
There was a problem hiding this comment.
Ca m'a pris un peu de temps à comprendre qu'il y avait 2 comportements possibles pour le constructeur selon ce que tu passais, c'est très intéressant !
Je suggérerai d'ajouter dans le premier test comme dans le second un
const std::filesystem::path new_config_path = "/tmp/test_config.conf";
Et éventuellement préciser les noms pour bien faire la différence de comportement
// In initializationConfig
const std::filesystem::path custom_full_path = "/tmp/test_config.conf";
// In initializationWithDefaultParentPathConfig
const std::filesystem::path custom_filename = "test_config.conf";| file.close(); | ||
| } | ||
|
|
||
| void spy_touchConfigFile() |
There was a problem hiding this comment.
touch filename c'est la commande pour créer un fichier sur unix
| spy_removeConfigFile(); | ||
| spy_touchConfigFile(); | ||
| auto data = configkit.read(config); | ||
| ASSERT_EQ(0, data); |
There was a problem hiding this comment.
Si le fichier ne contient aucun byte de donnée, c'est ConfigKit qui définit la valeur 0 par défaut?
There was a problem hiding this comment.
c'est une bonne question !
est-ce que pas de valeur == 0 ou == default_value.
je pencherai pour default_value puisque pas de fichier ou rien dedans c'est un peu la même chose --> la config n'existe pas vraiment
There was a problem hiding this comment.
Valeur = 0
Rien n'est lue effectivement mais la variable input est vide par défault (donc élément 0 par défault)
MMyster
force-pushed
the
mmyster/feature/config_kit
branch
2 times, most recently
from
10ed30c
to
4d016d2
Compare
MMyster
force-pushed
the
mmyster/feature/config_kit
branch
5 times, most recently
from
15de47f
to
805bd16
Compare
MMyster
force-pushed
the
mmyster/feature/config_kit
branch
from
805bd16
to
6b13e6e
Compare
MMyster
force-pushed
the
mmyster/feature/config_kit
branch
from
6b13e6e
to
fa962c2
Compare
ladislas
force-pushed
the
mmyster/feature/config_kit
branch
from
fa962c2
to
8e1ecbb
Compare
|
Kudos, SonarCloud Quality Gate passed! |
