Lazy configuration validator #124

karanankit01 · 2020-07-14T12:55:45Z

configuration field value validation

tammela · 2020-07-14T18:40:02Z

src/Config.cpp

@@ -43,6 +45,228 @@ Config::Config() {

    node_["double_quote_to_single_quote"] = false;
    node_["single_quote_to_double_quote"] = false;
+


You propagated the same lambda throughout this function. Why not a variable?

auto validate_integer = [](std::pair<std::string, std::any> elem) { if (strcmp(elem.second.type().name(), "i") != 0) { // Bug in the code assert(0); } int value = std::any_cast<int>(elem.second); if (value > 0) { return value; } std::cerr << "Integer value of " << elem.first << " is out of range" << std::endl; exit(1); }; validator["column_limit"] = validate_integer;

Let's not repeat code

tammela · 2020-07-14T18:43:26Z

src/Config.h

@@ -30,4 +32,6 @@ class Config {
    void set(const char *key, T value) const {
        node_[key] = value;
    }
+    typedef std::function<int(std::pair<std::string, std::any>)> Validator;
+    std::map<std::string, Validator> validation;


Suggested change

std::map<std::string, Validator> validation;

std::map<std::string, Validator> validators;

tammela · 2020-07-14T18:43:52Z

src/Config.h

@@ -30,4 +32,6 @@ class Config {
    void set(const char *key, T value) const {
        node_[key] = value;
    }
+    typedef std::function<int(std::pair<std::string, std::any>)> Validator;


Why a std::pair instead of two arguments?
Seems overkill to me.

tammela · 2020-07-23T11:37:27Z

src/Config.cpp

+    // Validators
+    auto validate_integer = [](std::string key, std::any elem) {
+        int value = std::any_cast<int>(elem);
+        if (value > 0) {


I would split this like you did because it might be desirable to have some configurations with 0s as well.

Suggested change

if (value > 0) {

if (value >= 0) {

And then make validators["spaces_before_call"] = validate_integer; as well

tammela · 2020-07-23T11:38:00Z

src/Config.h

@@ -30,4 +33,6 @@ class Config {
    void set(const char *key, T value) const {
        node_[key] = value;
    }
+    typedef std::function<std::any(std::string, std::any)> Validator;


Move this typedef to before the class declaration

tammela · 2020-07-23T11:39:39Z

test/test_validation.cpp

+        string configFileName(file);                                                                  \
+        Config config;                                                                                \
+        if (fs::exists(configFileName)) {                                                             \
+            std::cout << configFileName << " exist" << endl;                                          \


Suggested change

std::cout << configFileName << " exist" << endl; \

tammela · 2020-07-23T11:40:04Z

test/test_validation.cpp

+    TEST_CASE("Config file " + string(file) + " have error", "format_file") {                         \
+        string configFileName(file);                                                                  \
+        Config config;                                                                                \
+        if (fs::exists(configFileName)) {                                                             \


I don't understand this check, I think we can drop it

tammela · 2020-07-23T11:42:41Z

src/Config.cpp

+        if (value > 0) {
+            return value;
+        }
+        std::cerr << "Integer value of " << key << " is out of range" << std::endl;


Suggested change

std::cerr << "Integer value of " << key << " is out of range" << std::endl;

std::cerr << "[ERROR] Processing " << key << " failed";

tammela · 2020-07-23T11:44:47Z

src/Config.cpp

+            return value;
+        }
+        std::cerr << "Integer value of " << key << " is out of range" << std::endl;
+        throw domain_error("Config value is out of Range");


Suggested change

throw domain_error("Config value is out of Range");

throw domain_error("[ERROR] Configuration value is out of range. Must be a positive integer.");

tammela · 2020-07-23T11:46:16Z

src/Config.cpp

-                node_[key] = any_cast<bool>(kv.second);
-            } else if (strcmp(kv.second.type().name(), "c") == 0) {
-                node_[key] = any_cast<char>(kv.second);
+            if(datatype[key]== 'i'){


A switch statement here makes the code more readable.
Also don't forget to useclang-format

tammela · 2020-07-23T11:59:11Z

test/test_validation.cpp

+namespace fs = filesystem;
+
+#define TEST_CONFIG_FILE(file)                                                                        \
+    TEST_CASE("Config file " + string(file) + " have error", "format_file") {                         \


Suggested change

TEST_CASE("Config file " + string(file) + " have error", "format_file") { \

TEST_CASE("Configuration file " + string(file) + " has an error", "format_file") { \

tammela · 2020-07-26T11:58:45Z

Ok, time to do things right in the git history.
Squash all commits to one commit with the following message:
"initial implementation of validators"
Then I will re-review and you post the changes with fixup!

tammela · 2020-07-26T12:54:00Z

src/Config.cpp

+            return value;
+        }
+        std::cerr << "[ERROR] Processing " << key << " failed" << std::endl;
+        throw domain_error("[ERROR] Configuration value is out of range. Must be a positive integer.");


Suggested change

throw domain_error("[ERROR] Configuration value is out of range. Must be a positive integer.");

throw domain_error("[ERROR] Configuration value is out of range. Must be a positive integer greater than 0.");

tammela

@karanankit01 LGTM.
Please squash all commits using git rebase origin/master -i --autosquash
@Koihik please merge after squashing

tammela reviewed Jul 14, 2020

View reviewed changes

karanankit01 force-pushed the lazy branch from f0eec04 to 852d8a8 Compare July 15, 2020 20:42

tammela suggested changes Jul 23, 2020

View reviewed changes

karanankit01 force-pushed the lazy branch from b9c5adb to 1aae445 Compare July 26, 2020 12:09

tammela suggested changes Jul 26, 2020

View reviewed changes

tammela approved these changes Jul 28, 2020

View reviewed changes

karanankit01 force-pushed the lazy branch from e042849 to 0a3fa97 Compare July 29, 2020 05:26

initial implementation of validators

0a3fa97

Koihik merged commit acb531b into Koihik:master Aug 2, 2020

karanankit01 deleted the lazy branch August 3, 2020 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy configuration validator #124

Lazy configuration validator #124

karanankit01 commented Jul 14, 2020

tammela Jul 14, 2020 •

edited

Loading

tammela Jul 14, 2020

tammela Jul 14, 2020

tammela Jul 14, 2020 •

edited

Loading

tammela Jul 23, 2020

tammela Jul 23, 2020

tammela Jul 23, 2020

tammela Jul 23, 2020

tammela Jul 23, 2020

tammela Jul 23, 2020

tammela Jul 23, 2020

tammela Jul 23, 2020

tammela commented Jul 26, 2020 •

edited

Loading

tammela Jul 26, 2020

tammela left a comment

		@@ -43,6 +45,228 @@ Config::Config() {

		node_["double_quote_to_single_quote"] = false;
		node_["single_quote_to_double_quote"] = false;

	std::map<std::string, Validator> validation;
	std::map<std::string, Validator> validators;

	std::cerr << "Integer value of " << key << " is out of range" << std::endl;
	std::cerr << "[ERROR] Processing " << key << " failed";

	throw domain_error("Config value is out of Range");
	throw domain_error("[ERROR] Configuration value is out of range. Must be a positive integer.");

	TEST_CASE("Config file " + string(file) + " have error", "format_file") { \
	TEST_CASE("Configuration file " + string(file) + " has an error", "format_file") { \

Lazy configuration validator #124

Lazy configuration validator #124

Conversation

karanankit01 commented Jul 14, 2020

tammela Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tammela Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tammela commented Jul 26, 2020 • edited Loading

Choose a reason for hiding this comment

tammela left a comment

Choose a reason for hiding this comment

tammela Jul 14, 2020 •

edited

Loading

tammela Jul 14, 2020 •

edited

Loading

tammela commented Jul 26, 2020 •

edited

Loading