Support setting aribtrary k/v's in a task

[djmitche]

Currently, the only way to modify a task is to call a purpose-specific method like set_description() or set_status(). This is a nice high-level interface, but people like low-level stuff, too. In particular, TaskWarrior operates deeply at the k/v level, parsing or generating string values at the very "outside" of the application. So, it will want to have something like task.set_value(k, v). This should be documented as a "low-level" interface.

[miguelpduarte]

As per the discussion here I can take this one, if that's okay! 😄

[djmitche]

all yours :D

[djmitche]

@miguelpduarte how is this going?

[miguelpduarte]

• This issue mentions set_description and set_status as examples for functions that would be accessed via the to-be-implemented set_value(k,v). From what I see these functions usually accept structs as arguments instead of strings.

The idea is that some users of the taskchampion library may want to call set_value("description", updated_description) instead of calling set_description(updated_description). For example, if someone writes a web UI for Taskchampion, it might make sense to decide what task properties need to change in the browser, and just send those changes to the backend as property name and value. Without a set_value(k, v) function, the backend would have to have some kind of conditional:

match k {
  "description" => t.set_description(v),
  "status" => t.set_status(v),
  ...
}

Is it expected that this method would handle the casting of values (possibly Strings) to the appropriate structs? Or is there any more adequate pattern that I should consider using?

No, the underlying keys and values are always strings. So set_status does the conversion from a Status type to a string, but if the caller is using set_value(k, v), then it's up to the caller to generate the correct string v.

• Furthermore, to get a general idea, is the aim of this method to "delegate" the set operations to the respective set_x methods, or is it also necessary to operate on the underlying task at times?

No, quite the opposite -- this is a lower-level operation than set_x.

I see that several of these methods operate using self.set_string. Would it make sense to wrap this method instead? It appears that it already expects a property that can be Into'd to a String, so it seems like a more direct approach. However, I'm not sure of the impact of one approach vs the other, especially in terms of checking for potentially invalid values for some properties, etc.

Indeed, set_string is almost exactly what we want, but it has the side-effect of updating modified, which we would not want in this case. I suspect that when you've added set_value, set_string could be rewritten to just update modified and then call set_value.

• Finally, in regards to testing, should I only consider using tests under integration-tests, or are there any unit tests on the Rust side? (or even, should they be added regardless of not existing yet)

In Rust, the unit tests are typically in the same file as the code they test, enclosed in

#[cfg(test)]
mod test { .. }

and indeed, you'll see lots of unit tests in taskchampion/src/task/task.rs, and should add more.

I don't think there's any reason to add an integration test for this. Integration tests should be used sparingly for issues that cannot be checked by unit tests (such as whether two high-level components integrate correctly).