Closed dontlaugh closed 4 years ago
I did not write a test for this, but I intend to eventually. Testing this change is probably best suited to an integration test that works with a real file system, but I think we will need to get to the point where we can optionally render our task list to stdout as structured data (a json array, for instance).
Once we can do that, then we can parse the output of a test binary (with jq, with Go, with Python, even) and ensure that the ordering, the filtering, and the statuses of tasks rendered by the test binary matches our expectations. (this capability probably relates to #21 as well)
I'm marking this WIP, since I've found some time this weekend, and I'm going to try to bring in the rest of the filtering logic into this pattern. Here is what it will probably look like from commands.go.
@naggie This is ready for review.
The next thing I'd like to pick up is a rendering interface that supports multiple modes: json, non-tty stdout, etc. (#51 ) I do think it will help me write integration tests for dstask that have precise expectations about output.
Please forgive the eighty thousand line diff I just added in a single commit. Allow me to explain.
I have added a commit that renders JSON if stdout is not a tty.
To accomplish this, I have included a library that does this detection for multiple platforms: https://github.com/mattn/go-isatty The reason there is so much code is that doing this detection on multiple platforms requires importing a lot of platform specfic syscall code from subpackages of golang.org/x/sys/unix, so at least is is from a reputable source. The compiled binary on my linux machine is still only 5MB.
Now, the good news. Since we are not actually parsing output in smoketest.sh, all exit codes still return 0, so that still passes. If we provide JSON output, we are in a position to write integration tests with bash+jq, or Go.
I can also back the JSON rendering change out if we'd like to deal with that in a separate PR.
Note: the rendering strategy here is initially simplistic. If stdout is not a terminal, we print JSON. If stdout is a terminal, we print the table and messages as before (unless I've missed something in my refactor).
Hi @dontlaugh sorry I took so long to take a look. Thanks for the considerable effort
Could you separate out the JSON rendering / TTY handling to a separate PR please? It can be based against this branch. I think the sorting/filtering and JSON output are separate things making reviewing and discussion a bit difficult.
So as far as I can see, this part of the PR moves the filtering intent to the NewTaskSet function via the taskSetOpts mechanism; effectively making filtering declarative vs imperative. Is this as it feels like a natural progression of taskSetOpts? It seems natural to me.
Out of interest what are the advantages of doing it this way?
Edit: your comment here helps -- thanks. I'm on board with the reasoning.
I can see a use case of redirecting task notes to stdout, and appending from stdin. I'm not yet convinced of the utility of outputting JSON -- do we have non-speculative use cases? Is it better than using dstask as a library? (now possible thanks to your refactor, in fact a friend is working on a phone-capable dstask web app)
The isatty seems worthwhile and might also help with windows support for instance.
sorry I took so long to take a look
No worries!
Could you separate out the JSON rendering / TTY handling to a separate PR please
Sure thing :+1: I think I will actually just back this all out for now, as it was more experimental. I was pleased that it wasn't much code, aside from the giant import into vendor.
Regarding outputting JSON generally, I should clarify. I think this is something that the binary ought to do, but if I were authoring a sophisticated app in Go, I would want to use dstask as a library.
But not everyone will want to write a Go app, I reckon. Maybe they will write python, or bash scripts. So, for those use cases, it would be convenient if dstask provided "structured" output (if not json, specifically. yaml or toml or csv is also great), if requested.
In retrospect, the json stuff was a bit hasty, especially for the default behavior if stdout is not a tty. I'll back it out, and perhaps bring back the isatty library with a simpler use case: no changing the structure of output, but just printing the same table, but without color/ANSI escapes, in the service of something like #51
I reset this branch to just before the output stuff, so we back to just 4 files in the diff :sweat_smile:
I like how the context is merged with the command line query, for instance here: https://github.com/naggie/dstask/blob/c37a009a428db301945d1c3ac6876933a483e698/commands.go#L379
I think that's very clear. Happy to merge, thanks.
Thanks @naggie if you find any unexpected bugs due to this, tag me on them and I'll take a look
@naggie an aside, I have put your project on my profile https://github.com/dontlaugh I am hoping that, at this point, this is not presumptuous. I use dstask every day, and I intend to keep helping out as long as I keep using it! Thank you for this project!
@naggie an aside, I have put your project on my profile https://github.com/dontlaugh I am hoping that, at this point, this is not presumptuous. I use dstask every day, and I intend to keep helping out as long as I keep using it! Thank you for this project!
Cool! It's not presumptuous, I'm really grateful for the work you've put into it, you're welcome also!
We provide a default sorting strategy of P1 first, and then newest first, if no options are passed. Sorting is still performed by methods on TaskSet, but these methods are now private.
The order that SortBy options are passed matters, as it does in SQL.
Sorting happens to our TaskSet after tasks are loaded from disk, but before filtering happens. So for now, our procedure for building a TaskSet is LOAD -> SORT -> FILTER. Then we can render.