[BREAKING] Unify Task and TaskMut

[djmitche]

This breaking change removes the TaskMut type and moves its high-level setters to Task. Task derefs to TaskData, making the methods of that type available for Task values.

Several methods are deprecated, as detailed below. While these methods remain, they may be less performant than the recommended methods, either by reading unnecessary data from the database (e.g., to construct a dependency map) or by modifying the database in multiple transactions.

Breaking Changes:

• Task:

  • All TaskMut setters are now methods on Task, and take &mut ops as their last argument.
  • Task::into_mut is removed.
  • Task::delete is deprecated. Use Task::set_status with Status::Deleted instead.

• Replica:

  • Replica::add_to_working_set is removed - working set maintenance is now entirely automatic.
  • Replica::new_task is deprecated - prefer Replica::create_task and setting the entry, description, and status properties directly.
  • Replica::import_task_with_uuid is deprecated - prefer Replica::create_task.
  • Replica::update_task is deprecated - prefer TaskData::update.
  • Replica::delete_task is deprecated - prefer TaskData::delete.
  • Management of undo points in the replica, including Replica::add_undo_point and automatically adding undo points for various operations, is no longer supported; use Operations::new_with_undo_point to add one when necessary.

This is the first of two breaking changes for #372, which will necessitate a 0.7.0 release.

[ryneeverett]

Task derefs to TaskData, making the methods of that type available for Task values.

I haven't dug into it yet but I thought one of the premises of the refactor was to keep the high and low level interfaces distinct and that applications should be conscientious about which interface they're using. I'm not sure I still see this distinction as valuable with the current implementation, but without this distinction what value do we get from the extra wrapper layer of TaskData?

[djmitche]

It's possible to ignore the Task type entirely and just use TaskData, with the advantage that the DependencyMap is never calculated. But, if you want the convenience methods like set_status or add_annotation or is_blocking (which depends on the DependencyMap) and are thus using the Task type, there are still cases where the TaskData methods might be useful, such as task.properties or task.get_uuid. The Deref implementation lets the user call these TaskData methods directly on the Task value.

That reminds me: Task::delete shadows TaskData::delete. The former marks the task as deleted, while the latter actually removes the task from the DB with a Delete operation.

• pub fn Task::delete(&mut self, ops: &mut Operations) -> Result<(), Error>
• pub fn TaskData::delete(self, operations: &mut Operations)

I don't think this will cause confusion, and if it did it would be a shallow bug.

[djmitche]

Hm, and as I'm working on using https://cxx.rs/ to bind this into Taskwarrior:

• delete is a reserved word in C++ so it will need to be renamed, suggesting maybe renaming both methods might make sense
• The move receiver (self instead of &mut self) isn't compatible with cxx, leading me to take the task by reference (&self) and clone it. So maybe that's too clever.

[ryneeverett]

It's possible to ignore the Task type entirely and just use TaskData, with the advantage that the DependencyMap is never calculated. But, if you want the convenience methods like set_status or add_annotation or is_blocking (which depends on the DependencyMap) and are thus using the Task type, there are still cases where the TaskData methods might be useful, such as task.properties or task.get_uuid. The Deref implementation lets the user call these TaskData methods directly on the Task value.

Yes, but as a Task user, don't you need to be conscientious when using TaskData methods since they don't commit operations? Doesn't the Deref make it less obvious that you're using the low-level interface and need to take responsibility for committing operations?

[djmitche]

The new Task methods do not commit either: like Task data, they add operations to an &mut Operations.

But the Deref may not add a lot of value for users. Should I remove it?

[djmitche]

I'll make a few changes here:

• Remove the Deref/DerefMut implementations. While I don't think users would need to be conscientious, I also don't think it adds much value. These were widely used internally so this is a nontrivial change.
• Deprecate Task::delete. It's just a simple wrapper around task.set_status(Status::Deleted) so users can simply do that.

[ryneeverett]

The new Task methods do not commit either: like Task data, they add operations to an &mut Operations.

I don't understand how this is the case. Pretty much all the Task methods call set_value which calls self.replica.commit_operations(ops)?; at the end.

Edit: I now see that this PR removes this line!

[ryneeverett]

What is the rationale for this change?

[djmitche]

IIRC,it was so I could call Ok(Task::new(TaskData::create(uuid, ops), depmap)) without having to de-construct and re-construct that TaskData.

[djmitche]

Edit: I now see that this PR removes this line!

You had me really worried & confused :)