From 96c72f3e06e5d33f29057e18c89afd61a53ddb1c Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 22 Oct 2024 15:15:51 -0400 Subject: [PATCH] Issue warnings instead of errors for 'weird' tasks (#3646) * Issue warnings instead of errors for 'weird' tasks * Support more comprehensive checks when adding a task --- src/TDB2.cpp | 4 ++++ src/Task.cpp | 48 ++++++++++++++++++++++++++------------ src/Task.h | 1 + src/commands/CmdImport.cpp | 3 +++ test/import.test.py | 4 ++-- 5 files changed, 43 insertions(+), 17 deletions(-) diff --git a/src/TDB2.cpp b/src/TDB2.cpp index 2240c5213..d5dacd769 100644 --- a/src/TDB2.cpp +++ b/src/TDB2.cpp @@ -57,6 +57,10 @@ void TDB2::open_replica(const std::string& location, bool create_if_missing) { //////////////////////////////////////////////////////////////////////////////// // Add the new task to the replica. void TDB2::add(Task& task) { + // Validate a task for addition. This is stricter than `task.validate`, as any + // inconsistency is probably user error. + task.validate_add(); + // Ensure the task is consistent, and provide defaults if necessary. // bool argument to validate() is "applyDefault", to apply default values for // properties not otherwise given. diff --git a/src/Task.cpp b/src/Task.cpp index 6474c4384..6c51dedbe 100644 --- a/src/Task.cpp +++ b/src/Task.cpp @@ -1408,13 +1408,29 @@ void Task::substitute(const std::string& from, const std::string& to, const std: } #endif +//////////////////////////////////////////////////////////////////////////////// +// Validate a task for addition, raising user-visible errors for inconsistent or +// incorrect inputs. This is called before `Task::validate`. +void Task::validate_add() { + // There is no fixing a missing description. + if (!has("description")) + throw std::string("A task must have a description."); + else if (get("description") == "") + throw std::string("Cannot add a task that is blank."); + + // Cannot have an old-style recur frequency with no due date - when would it recur? + if (has("recur") && (!has("due") || get("due") == "")) + throw std::string("A recurring task must also have a 'due' date."); +} + //////////////////////////////////////////////////////////////////////////////// // The purpose of Task::validate is three-fold: // 1) To provide missing attributes where possible // 2) To provide suitable warnings about odd states -// 3) To generate errors when the inconsistencies are not fixable -// 4) To update status depending on other attributes +// 3) To update status depending on other attributes // +// As required by TaskChampion, no combination of properties and values is an +// error. This function will try to make sensible defaults and resolve inconsistencies. // Critically, note that despite the name this is not a read-only function. // void Task::validate(bool applyDefault /* = true */) { @@ -1428,6 +1444,8 @@ void Task::validate(bool applyDefault /* = true */) { Lexer lex(uid); std::string token; Lexer::Type type; + // `uuid` is not a property in the TaskChampion model, so an invalid UUID is + // actually an error. if (!lex.isUUID(token, type, true)) throw format("Not a valid UUID '{1}'.", uid); } else set("uuid", uuid()); @@ -1543,27 +1561,27 @@ void Task::validate(bool applyDefault /* = true */) { validate_before("scheduled", "due"); validate_before("scheduled", "end"); - // 3) To generate errors when the inconsistencies are not fixable + if (!has("description") || get("description") == "") + Context::getContext().footnote(format("Warning: task has no description.")); - // There is no fixing a missing description. - if (!has("description")) - throw std::string("A task must have a description."); - else if (get("description") == "") - throw std::string("Cannot add a task that is blank."); + // Cannot have an old-style recur frequency with no due date - when would it recur? + if (has("recur") && (!has("due") || get("due") == "")) { + Context::getContext().footnote(format("Warning: recurring task has no due date.")); + remove("recur"); + } - // Cannot have a recur frequency with no due date - when would it recur? - if (has("recur") && (!has("due") || get("due") == "")) - throw std::string("A recurring task must also have a 'due' date."); - - // Recur durations must be valid. + // Old-style recur durations must be valid. if (has("recur")) { std::string value = get("recur"); if (value != "") { Duration p; std::string::size_type i = 0; - if (!p.parse(value, i)) + if (!p.parse(value, i)) { // TODO Ideal location to map unsupported old recurrence periods to supported values. - throw format("The recurrence value '{1}' is not valid.", value); + Context::getContext().footnote( + format("Warning: The recurrence value '{1}' is not valid.", value)); + remove("recur"); + } } } } diff --git a/src/Task.h b/src/Task.h index 08d508f0a..f36e79b83 100644 --- a/src/Task.h +++ b/src/Task.h @@ -164,6 +164,7 @@ class Task { void substitute(const std::string&, const std::string&, const std::string&); #endif + void validate_add(); void validate(bool applyDefault = true); float urgency_c() const; diff --git a/src/commands/CmdImport.cpp b/src/commands/CmdImport.cpp index 8c7aa3486..31a2e141a 100644 --- a/src/commands/CmdImport.cpp +++ b/src/commands/CmdImport.cpp @@ -174,6 +174,9 @@ void CmdImport::importSingleTask(json::object* obj) { // Parse the whole thing, validate the data. Task task(obj); + // An empty task is probably not intentional - at least a UUID should be included. + if (task.is_empty()) throw format("Cannot import an empty task."); + auto hasGeneratedEntry = not task.has("entry"); auto hasExplicitEnd = task.has("end"); diff --git a/test/import.test.py b/test/import.test.py index 7771345a3..1c709c23d 100755 --- a/test/import.test.py +++ b/test/import.test.py @@ -284,10 +284,10 @@ class TestImportValidate(TestCase): self.t = Task() def test_import_empty_json(self): - """Verify empty JSON is caught""" + """Verify empty JSON is ignored""" j = "{}" code, out, err = self.t.runError("import", input=j) - self.assertIn("A task must have a description.", err) + self.assertIn("Cannot import an empty task.", err) def test_import_invalid_uuid(self): """Verify invalid UUID is caught"""