-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Task-list format option #33
Comments
See #57 which is trying to implement this ( |
See also #104 which doesn't entirely obviate this but does intend to implement one major other "list format" (machine-readable). |
See also #379 which is a strongly related request for "list just the namespaces", "show namespace docstrings when listing (vs task docstrings when doing --help)" and so on. |
Starting out with the obvious approach, e.g:
Still not sure how to handle namespace stripping (eg. This stuff should end up more refactored and flexible just by virtue of implementing all the rest, so adding on a strip option should be relatively painless if enough users clamor for it. |
Also pondering whether use of |
Also thinking the nested display option probably wants to not bother with the first-line-of-docstring column, since it degrades poorly once nesting gets too deep (e.g. trying to wrap the docstring line into a narrow column would eventually get ridiculous and take up far too much vertical space.) In an ideal world we'd just borrow a page from responsive web UX design and have the docstring column disappear at that point, but I don't have that kind of time right now :( The base case of only a few nested levels, would look nice w/ the docstring column, but sacrifices must be made for now. |
Also, the JSON format "wants" to be very rich and contain fully serialized JSON copies of Task and Collection objects (so it could be used to e.g. power external completion or presentation code that can't actually call But in the interests of expediency (and also because serialization is never actually simple to do right, and also adds more friction re: changing these classes' APIs) I'm sticking to largely the same data that is exposed in the other formats: task and collection names, docstrings, and relationships. As always we can add more to this later if folks really need it. |
While I'm conversing with myself: contemplated whether it was worth mentioning these new flags in the getting-started document, but I don't think it is, feels like needless extra mental overhead. Seems appropriate for it to remain discoverable via Maybe worth mentioning in the namespace docs but a lot of that is due a reorg anyways so might wait for then. |
+10!
My preference would be to start out with the --list implication from the get-go.
SGTM.
I think so too. After all, there's likely a statistically significant percentage of invoke users that only have list depth of 1 anyway. More complicated use cases will require a little docs reading. |
Grumble, so by innocently trying out the optional-value form of I implemented optional-value flags in #57 and since then I've personally only really used it for flags that tend to be given by themselves, such as the original case of Now, though, we not only want, but need, the ability to invoke things like Clearly, in a general sense, it's impossible to globally "fix" the problem because any string-wanting optional-value argument could want a very argument-like string value, up to and including truly valid-for-this-parser-context arguments. However, my gut is telling me that when the ambiguous value is an otherwise valid-for-this-parser-context token (so, another flag, or the name of another possible parser context aka task) it might be better to ease off on the ambiguity freaking-out. In such situations, it really seems far more likely the user is trying to do something like I'm trying to do above, than that they're screwing up. Need to dig a bit to see if I can remember what case I had where that situation still wanted the hardcore refuse-to-guess behavior...worried if I change this it'll tank something else important. |
I guess another "solution" here is for the user to remember to give an optional-value flag at the end of the invocation, thus removing the ambiguity that way - but that seems kinda shitty to me. Even if it's tempting to go "good enough, you have a deadline" I can just see a lot of users trying to use the new shiny list functionality and running headlong into this. Esp since intuitively one kinda wants to give EDIT: spun that off to #516. |
Weird side issue I noticed while testing, I think we go a bit too hard on the breadth-first organization of our default/flat format. e.g. here's the early fixture data I am working with right now (gonna change it up though, it's too irritating to work with even for me):
Notice how the Less immediately obvious flat, but as soon as you start thinking about the tree format, it jumps out at you. I honestly can't think why one would really prefer the way it looks right now - yes, having all the very top level tasks "up top" is nice but that aspect doesn't actually work as well when it recurses. May roll a rework of that into this - but not sure, it is nice having the default list output stay backwards compatible...and now we'll have a tree view for folks that are bothered by the breadth-first list view. Eh. |
Overnight epiphany, this ought to get implemented as something vaguely pluggable so it's easier to add more later or for users to add their own w/o needing a PR. Less pressure to get it 100% right now too... |
Fleshing out the tests more, I realize that the tree view can actually end up being less wide in the name-of-tasks column, versus the flat view - because a constant sized 2 or 4 space indent is almost always less than the name of the outer collections! E.g. I think the only violation of this is how flat view currently denotes default tasks per collection (as So I think having docstrings in both views is probably actually fine for now... |
Quick brainstorm re: tree output format for posterity (I'm probably just gonna pick whatever looks best to me offhand, for expediency, but as above I could see us switching it again later or whatever.) My first off the cuff approach was 2 space indents, no dots:
After my epiphany number 2 above, I made it 4 space indents since saving horizontal space seemed less necessary:
Think that looks a bit nicer, and it doesn't even take up THAT much more room since the deepest nest level tends to 'absorb' extra indents for anything below it. Then I figured, what about making it more obvious that the subtasks cannot be called on their lonesome but must be specified in the dot syntax? (Something the flat style has no problem with...) Could either put dots after collections, or before subtasks. Before subtasks seems slightly harder to miss/misinterpret...maybe?
For comparison, here's the dots-after-collections version:
|
My gut is saying the leading dots, while slightly noisier, is more idiot-proof and less ambiguous. The trailing dots look like the asterisks I'm using for 'default task for namespace' and might lead folks to wonder if they signify "some other special kind of task" instead of "a collection". It's also tempting to rub some ANSI on here too, given it's intended solely for human evaluation (anything not so, would want the upcoming JSON output.) E.g. bold collection names. Not sure. |
Alias display also presents some minor issues, e.g. do they also get leading dots or not? If they do, e.g. For now I'm going with consistency and putting dots everywhere; I suspect aliases aren't used terrifically often anyways and we can easily tweak some of this later. (Again, being human facing and providing a computer-facing alternative means we don't even really have to consider minor changes to this stuff as breaking backwards compat.) |
+1
-1 It wouldn't seem proper to be valid. Seems more intuitive to my brain that the trailing asterisk indicates running all commands in that namespace / collection. |
That's one minor issue with my choice of fixture data - the "default task does all the things" pattern is (IME) common, but is not actually a property of what it means to be a default task for a collection. It's entirely plausible to have a collection of tasks where the default task just does its own thing, runs some non-full subset of the other tasks in the collection, or etc. So when I was talking about 'all' vs 'everything' and the dots, I just meant that if the default task has an alias, and we display both the primary name and the alias on the same line, both with leading dots - it starts to resemble a nested task path that doesn't really exist. But this is minor I think. |
Cool, nested format base case is implemented, e.g. here's it on Invoke's internal task tree:
Onwards and upwards to the depth & root selectors... |
Also this reminds me I really need to figure out what happened to trimming trailing underscores in task names. |
Re: namespace/root-filtered display, gonna follow the lines above re: tasks having an explicit leading dot to indicate they are part of a dotted path - so the output will look like this:
Or possibly this, making it SUPER explicit that this is a display/list filter & not a full-on re-rooting your Invoke experience at the given subcollection:
But not e.g. this (note no leading dots at all, which could mislead folks into thinking they can just call
Though I like the idea of the above; and it strongly suggests we want to accompany, or merge, the existing (That has a whole host of non-obvious follow-ons for features extant and upcoming, so I'm not going down that rabbit hole today, but just FTR...it feels like the sort of intuitive thing folks might expect.) |
One downside with option 1 vs 2 above is that it's less consistent - w/o any alternate root, all task paths are fully explicit, and you need to use nested format to 'factor' things out. But then we give an alternate root, and suddenly, poof, we're no longer seeing fully explicit paths. However, most arguments I can think of for emphasizing consistency over "it looks nice / is readable / is obvious" boil down to "what if I was scripting something that is reading this?" - and as I've said a few times in here, one should be using the JSON format for that now. |
Heh, another consequence of the "nice" factoring out of the given root, is that default tasks have no immediately obvious solution:
But now that I am rubber ducking, I realize, aha: the obvious solution is to acknowledge that the current context is some specific collection, so we can just pull this out of the list entirely if we want - same as we probably should be doing for the top level default task! (Which I don't think we currently highlight at all?) E.g. stick it in the header line:
Or on its own line between header and listing (tho then the colon is messy), or at the bottom of the listing ( Gotta think about it. |
Well, but not for long because I seriously don't have time for these fascinating weeds. Putting it in the header does seem like the objectively cleanest approach for now, so I'mma do that. GRUMBLE: except that kinda clashes with the note about asterisks in nested view, making the line pretty long; and also just means I gotta update a ton of tests (heh). The other idea is to put some dummy thing in, like Maybe moving all the extra bits to the footer is best - only some tests have to change that way, it avoids piling too much into the header, and it's still pretty readable? |
Not crazy happy with it (so many little aspects tugging in different directions!) but got that mostly knocked out - can now root to any part of the namespace and the flat + nested views both respond appropriately. Plays nicely with default tasks. Etc. What's left is the depth limiter, and JSON format, and I'm done?! |
JSON's done. Not super duper happy there either (eg ran into ye olde "list of objects with We can always beef that up more later if it seems desirable, though I'd assume anyone wanting to do deeper things on their own with collections/tasks would hopefully be wanting to just use the in-Python APIs. |
As with most other new features, even depth is not as simple as it sounds:
As for a "wholly different approach", I could throw out the idea of depth being a nice simple display dimension like format, and make an entirely separate listing command like @thinkwelltwd (or anyone else reading this) any thoughts here? I'm gonna go work on some other (more rote) tasks while this simmers in my brain, and extra input is welcome. |
Still very on fence about this but gotta pick a road to go down. Hell, just writing this comment I've deleted like 3 different explanations of the pros/cons of either option. Will see what I can execute on quickest, we can always change later, now that JSON is a thing. |
Also the way that current I'm honestly tempted to axe the flat view entirely, except for the super nice relation to the actual dotted paths one has to use to invoke the tasks. A second worst might be to make nested the default (but then we have this vestigial, arguably buggy other format lying around). Or I could just try to fix the way the flat format is sorted. Esp now that nested exists, trying to be helpful by 'surfacing' closer-to-root tasks towards the top of the list, seems less useful. (Plus then I believe the two lists would be in the exact same order, just differently presented, which is nice...) |
I don't have specific suggestions to offer. But in general terms, I'd say favor the simplest implementation, requiring the user-specified fewest parameters. If it starts itching on really deep nesting, then the library user can always broaden instead of deepen their tasks / collections. When that's no longer an option, there will likely be more details about how to improve presentation of namespaces & collections.
+1 Nested view is so much more intuitive and scalable. IMO, anything that hinders the implementation of nesting is worth eliminating. But then everybody's tastes vary so perhaps fixing flat format sorting would be better. If both flat/nested are retained, I agree the sorting should be identical. |
Thanks for the input :D I did manage to fix the flat sorting last night, just never posted about it. So that's nice and it does make the depth option probably work better. Will tackle that next/finally. |
Side note as I wrap this up, I don't see an obviously intuitive way to handle the conjunction of JSON format and the depth arg; the only thing that springs to mind is aping what I am doing with the other formats and changing the value of a truncated collection so its Just feels kinda wrong to me, esp insofar as depth limiting is more of a human facing "I cannot deal with too much information" and JSON format is arguably for computer consumption - that consumer should presumably be able to deal with truncating its display of the data on its end, if it's desired. Also I'm lazy and running out of time. So I'm gonna have it just spit out an error instead of returning arguably-confusing truncated JSON data, for now. Can always enhance later. |
Wow...it's done?! Jeez. Took longer than anticipated, as always, though I wasn't working on it at all last week. Leaves some older flat-oriented task list code mostly vestigial (eg Gonna do the CI dance and then pop this out as whatever the next pre-1.0 release number is...along a handful of other things that I think are sitting in the pipe. |
I.e. instead of just
inv --list
spitting out the current default flat list, allow things likeinv --list nested
for a nested/indented display,inv --list=foo
to just list the tasks within namespacefoo
, etc ad infinitum.LATER EDITS: so a lot of strongly related tickets have popped up and there seem to be two major currents:
Seems unwise/impossible to shoehorn all of that under a single optional value to
--list
so I'm sure we'll grow some additional flags either way, with the main question being which (if any) such option deserves to live as the optional--list
value.Brainstorm:
--list-format (flat|nested|machine)
(what term do other tools use for "machine-readable" anyways? Can't remember offhand...LATER EDIT: actually these days we often just use JSON for that and just call it eg--list-format=json
) or it could be a handful of booleans like--list-nested
,--list-machine
.--root
flag which changes where tasks files are sought from; but perhaps we should be, well, namespacing those as well so they are renamed to eg--discovery-root
) and 'depth` and I think that's it, since those together can enable just about anything:inv --namespace foo --list
spitting out task names likebar
andbaz
instead offoo.bar
andfoo.baz
) should be relying on "each Python module can be its own standalone namespace" and doing things likeinv --collection=namespace --list"
instead (see Collection loading.)--collection
automatically ensures that everything - display, invocation, etc - instantly works as you'd expect.foo.bar
as an argument to--collection
- even though, between you and me,foo.bar
isn't visible on the Python import paths."(root)
,foo
andbar
"The text was updated successfully, but these errors were encountered: