TaskSet and func main refactor

[dontlaugh]

A few times now we've hinted at a desire to refactor func main. This will involve TaskSet as well, since manipulating this type is one of main's big jobs right now.

We can talk about some possible designs here. We'll want something testable and extensible, if possible. For instance, how might we accommodate #12 and/or #16 ? We don't want to come up with something that makes those awkward to implement.

[dontlaugh]

An analysis of our current commands yields the following

Commands that update state on disk

• add
• remove
• template
• log
• start
• stop
• done, resolve
• context
• modify
• edit
• note
• undo
• git
• import-tw

Commands without side effects

Note that the sync command does change the state of the world, technically. But, from the perspective of the local task database, there are no changes.

• sync
• show-active
• show-paused
• show-open
• next (also the default command)
• show-projects
• show-tags
• show-templates
• show-resolved
• show-unorganised
• help
• version

[dontlaugh]

We might consider a separation of responsibilities like this:

func main()

• reads configuration from the environment (i kind of include 'Context' in this category of runtime config)
• parses command line to determine which command is being run
• calls that command routine
  • commands might be utilities, such as completion script generators
• returns an exit code to the shell

TaskSet

• Constructor reads a subset of tasks from disk (depending on options, such as "status")
• This subset of tasks is held in memory, and we either a) make changes: add new task, start/stop, add notes, persist those changes to disk, etc, then render sensible output b) make no changes, construct a table for rendering, render the output

Changes:

• set context on TaskSet so it can control rendering of current context (usually is printed before the task table)
• add a NewTaskSet constructor that takes more options than LoadTasksFromDisk. We could use the functional options pattern, to allow accepting a variadic list of options.

  
  func NewTaskSet(repoPath string, opts ...TaskSetOpt) (*TaskSet, error) {
  var tso taskSetOpts
  for _, opt := range opts {
  opt(&tso)
  }
  // if tso.This or tso.That do stuff
  }

type TaskSetOpt func(opts *taskSetOpts)

func WithStatuses(statuses ...string) TaskSetOpt { return func(opts *taskSetOpts) { opts.statuses = append(opts.statuses, statuses...) } }

type taskSetOpts struct { statuses []string renderStrategy string }

// ResolveTasks gets a list of tasks by id, marks them resolved, and commits the result. func (ts *TaskSet) ResolveTasks(ids ...int) error { ... }

* add a TaskSet methods for `AddTask`, `RemoveTask`, `StartTask`, etc. All state mutations are moved inside the task list
* return errors from everything outside `main`

From `main`, it might look something like this (ignoring some errors)

```go
case dstask.CMD_RESOLVE:
  ts, _ := dstask.NewTaskSet(repoPath, dstask.WithStatuses(dstask.NON_RESOLVED_STATUSES...))
  if err := ts.ResolveTasks(cmdLine.IDs...); err != nil {
    dstask.ExitFail("aborting: %v", err)
  }
  // we have been given enough options to know how to render
  ts.Render()

Advantages of this design:

• We lean into the existing design. TaskSet is already managing some in-memory state, and has an ORM-like functionality. We extend this to reading and writing from disk, and task manipulation
• LoadTasksFromDisk is really a constructor for TaskSet. We can make this more powerful, get rid of its dependency on global state, and return more detailed errors

Disadvantages:

• TaskSet becomes godlike. While technically it is a testable object, at this point it might better be named Application, even.

---

Alternatively, we could opt to keep the new constructor, but not full embrace encapsulating git commits and disk persistence within named methods of TaskSet. We could, for instance, do something like the following:

• add a new file in root directory, commands.go
• This file contains plain functions, one per command. For example,

func ResolveTasks(repoPath, cmdLineText string, ids ...int) error {
  ts, _ := NewTaskSet(repoPath, dstask.WithStatuses(NON_RESOLVED_STATUSES...))
  for _, id := range ids {
    task := ts.MustGetByID(id)
      task.Status = STATUS_RESOLVED
      if cmdLineText != "" {
      task.Notes += "\n" + cmdLineText
    }
    ts.MustUpdateTask(task)
    ts.SavePendingChanges()
    MustGitCommit("Resolved %s", task)
  }
}

Advantages:

• still makes main shorter, eases us into the path of returning errors from everything
• doesn't commit to any state management strategy, doesn't preclude us from doing anything on that front at all

Disadvantages:

• Doesn't offer any opinion or strategy around rendering, punts on that until later (but that's okay)

[naggie]

Sorry I took so long to review this, struggling to find the time at the moment! Thanks for putting the time into thinking about a refactor, we certainly need one now that we're adding commands and there's some redundancy in the big switch statement.

I like the idea of setting a context on taskset, it simplifies things and is similar to the existing filter mechanism.

The alternative suggestion was actually exactly what I had in mind myself, adding commands which are implemented as functions, removing the logic from cmd.go which now mostly just does the translation between the CLI world and Go. Perhaps it strikes the right balance without a single godlike class which I'd rather avoid. We can also return errors from these functions as suggested, perhaps with a single error handler in cmd.go.

EDIT: Either change would make it easier to use dstask as a library, which would be useful for making an iOS or Android app. We'd have to switch to a go implementation of git -- though I think that would be fairly painless as we're not doing anything too strange.

[dontlaugh]

I think option number 2 is what I'd advocate, perhaps implemented in two phases. 1) introduce the new constructor for TaskSet, and 2) then move a lot of func main code into a commands.go.

I might give it a shot this weekend. But before that I'd like to help @ard0gg with testing their PR #37 since I haven't gotten too familiar with the completions code. And it would be good to land that before refactoring out from under them.

[naggie]

Agreed -- thanks, sounds good!

[dontlaugh]

I ended up opening #43 for both initial phases, moving much of func main to commands.go and introducing a new constructor for TaskSet.

[naggie]

@dontlaugh are you happy to close this now, or do you think further work is required?

[dontlaugh]

@naggie I think we can close this in favor of a new issue around sorting, and the existing issue #12 . I very recently took a look at next steps for folding in more logic into the TaskSet constructor with associated options. I think the "big one" is sorting.

In the constructor, we might apply sorting after we've read things from disk with something like: https://github.com/gloriousfutureio/dstask/commit/d7aca9439b0fa37b8bce0c44376688ba8200685f

The more I look at it, the more it looks like we're building a query interface to the task database. Given state (in the repo) + some well structured inputs, we should be able to read in and construct a sorted and filtered list of tasks in one go.

I'll take on sorting next, since it's on my mind and is an existing feature.

I also think it is possible to handle #12 within this design. It is an interesting problem. Time is always a bit tricky.