Skip to content
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

Apply --depth flag to --tasks-json flag output #57

Closed
phated opened this issue Jan 31, 2016 · 28 comments
Closed

Apply --depth flag to --tasks-json flag output #57

phated opened this issue Jan 31, 2016 · 28 comments
Milestone

Comments

@phated
Copy link
Member

phated commented Jan 31, 2016

In 1.2.0, @sttk added a --depth flag to gulp-cli; However, we didn't apply it to the --tasks-json flag to reduce the exponential growth as described in gulpjs/gulp#1033

@sttk If you or anyone else would like to expand on the functionality you added for --depth and apply it to --tasks-json, that would be super helpful.

@sttk
Copy link
Contributor

sttk commented Feb 2, 2016

@phated I'm willing to tackle this also, but I have a question.

There are cases that --depth cuts child tasks of series/parallel tasks. Are the json data of such series/parallel tasks useful?

I think --tasks-json should be able to output in two type forms, a full tree and a tree until first child tasks which are not series/parallel tasks. And the latter form would be possible if gulp-cli can get branch flags in undertaker.

@sttk
Copy link
Contributor

sttk commented Feb 3, 2016

However, it may be better that --tasks and --tasks-json can output same trees by --depth. How do you think about my idea below?

(default or --depth=full)
[07:09:11] ├─┬ task0
[07:09:11] │ └─┬ <series>
[07:09:11] │   ├─┬ <parallel>
[07:09:11] │   │ ├── func1
[07:09:11] │   │ └── func2
[07:09:11] │   └── func3
[07:09:11] └─┬ task1
[07:09:11]   └─┬ <series>
[07:09:11]     └─┬ task0
[07:09:11]       └─┬ <series>
[07:09:11]         ├─┬ <parallel>
[07:09:11]         │ ├── func1
[07:09:11]         │ └── func2
[07:09:11]         └── func3
(--depth=3)
[07:10:10] ├─┬ task0
[07:10:10] │ └─┬ <series>
[07:10:10] │   ├── <parallel>
[07:10:10] │   └── func3
[07:10:10] └─┬ task1
[07:10:10]   └─┬ <series>
[07:10:10]     └── task0

(--depth=4)
[07:11:37] ├─┬ task0
[07:11:37] │ └─┬ <series>
[07:11:37] │   ├─┬ <parallel>
[07:11:37] │   │ ├── func1
[07:11:37] │   │ └── func2
[07:11:37] │   └── func3
[07:11:37] └─┬ task1
[07:11:37]   └─┬ <series>
[07:11:37]     └─┬ task0
[07:11:37]       └── <series>
(--depth=nonrecursive, or another name. I didn't find an appropriate name.) 
[07:11:37] ├─┬ task0
[07:11:37] │ └─┬ <series>
[07:11:37] │   ├─┬ <parallel>
[07:11:37] │   │ ├── func1
[07:11:37] │   │ └── func2
[07:11:37] │   └── func3
[07:11:37] └─┬ task1
[07:11:37]   └─┬ <series>
[07:11:37]     └── task0

@phated
Copy link
Member Author

phated commented Feb 3, 2016

@sttk thanks for working on this. All the information I have is at gulpjs/gulp#1033 - I'll cc @segrey here to give more insights on how he needs this to be for IDE integration. Thanks again.

@sttk
Copy link
Contributor

sttk commented Feb 5, 2016

First, I've modified that --tasks-json outputs same trees as current --tasks using --depth flag.

sttk/gulp-cli#modify_for_tasks_json

Modifications:

  • copyTree was separated from logTasks except sorting tasks and node creations.
  • changed the processing for --tasks-json in each index.js of the versions to use copyTree and to support --depth flag.
  • added tests for copyTree.
  • added a test case to taskTree.js. This case shoud have been added at the time of the modification for --tasks.

@sttk
Copy link
Contributor

sttk commented Feb 6, 2016

In addition, I've added modifications to support --depth=nonrecursive.

sttk/gulp-cli#modify_for_tasks_json_norcr

But this program identifies series/parallel tasks by their labels <series> or <parallel> tentatively. For this I want to modify undertaker to include branch flags in its task tree output. Can I do it?

@phated
Copy link
Member Author

phated commented Feb 6, 2016

@sttk at first thought, that should be fine.

@segrey
Copy link

segrey commented Feb 6, 2016

@sttk thanks for working on this. Seems --depth=nonrecursive does exactly what's needed. As for naming, maybe --depth=compact?

@sttk
Copy link
Contributor

sttk commented Feb 7, 2016

Thank you for replies.

@phated Please tell me problems you think about my second thought above.

@segrey I've changed the naming of --depth=nonrecursive to --depth=compact.

@phated
Copy link
Member Author

phated commented Feb 8, 2016

@sttk does it make sense to just make everything non-recursive and change the color or use some other indicator (maybe @tylersticka could help us brainstorm that design). There are a few issues, including #1410 and #1033, that mention this problem. Also, I don't really like making a numerical flag take some other value.

@tylersticka
Copy link

Boy, this is a tough problem to solve! I'm using @phated's example from the other issue as a point of reference.

I initially took @phated's suggestion and tried playing with color, but the issue is that if you have two gulp.series or gulp.parallel groups one right after the other, there's no way to differentiate where one group stops and the other ends. Also, the child tasks end up kind of overpowering anything else... I really like the look of cool colors like cyan and purple for task names and such, but they are overthrown easily by any warmer colors in the palette.

My next experiment was a play with what @sttk recommended. The only changes are that I like parentheses more than less-than/greater-than signs (there are already so many hard angles in this output), and I started playing with different box-drawing characters to differentiate parallel tasks from series:

screen shot 2016-02-08 at 4 24 03 pm

[18:08:06] └─┬ combined      Build all the things!
[18:08:06]   │ --dev         …un-minified
[18:08:06]   │ --production  …compressed into single bundle
[18:08:06]   ├── clean
[18:08:06]   ├─╥ (parallel)
[18:08:06]   │ ╟── js
[18:08:06]   │ ╟── css
[18:08:06]   │ ╙── img
[18:08:06]   └─┬ (series)
[18:08:06]     ├── some
[18:08:06]     ├── other
[18:08:06]     └─╥ (parallel)
[18:08:06]       ╟── thing
[18:08:06]       ╙── stuff

This works fine, though I'm not sure the different drawing characters are communicating very much... the headings say everything on their own, really.

Then I wondered if maybe I could eliminate those (parallel) and (series) headings entirely, retaining the indentation and pushing the parallel line characters further:

screen shot 2016-02-08 at 4 33 14 pm

[18:08:06] └─┬ combined      Build all the things!
[18:08:06]   │ --dev         …un-minified
[18:08:06]   │ --production  …compressed into single bundle
[18:08:06]   ├── clean
[18:08:06]   ╞═╦══ js
[18:08:06]   │ ╠══ css
[18:08:06]   │ ╚══ img
[18:08:06]   └─┬── some
[18:08:06]     ├── other
[18:08:06]     ╘═╦══ thing
[18:08:06]       ╚══ stuff

I like the way this looks more, but it may be harder to read... I can't tell if it's clear what the series versus parallel groupings are. Would love to get some more opinions.

@phated
Copy link
Member Author

phated commented Feb 9, 2016

It's not too clear what the parallel and series tasks are. Maybe (series) or (parallel) could come after the top level task name? e.g. build (series)

@sttk
Copy link
Contributor

sttk commented Feb 9, 2016

@phated ...I might misunderstand "at first thought" in your saying.

I received your saying that first thought, which is to apply current --depth flag, was fine (but second thought, which is to add --depth=non-recursive, was not fine). Was that meaning like "at first glance" or "at first sight"? (Or didn't I misunderstand? You said "I don't really like ...".)

...Sorry. My english skill is poor.

@sttk
Copy link
Contributor

sttk commented Feb 9, 2016

Should I send a PR of #modify_for_tasks_json, or add a new option for non-recursive?

@phated
Copy link
Member Author

phated commented Feb 9, 2016

@sttk sorry, I meant surfacing the branch flag would be fine. I'd like to figure out if we can make "non-recursive" the default and only behavior before we go about adding it as part of the flag. If we can communicate everything concisely in the --tasks UI in a non-recursive manner, I'm okay with making that the only way things are displayed.

@segrey
Copy link

segrey commented Feb 9, 2016

@phated I think it makes sense to have the only behavior. Alternative idea could be to have --tasks-json=top (without dependencies of top level tasks) and --tasks-json=full (the same as non-recursive). The same can be applied for --tasks.

@phated
Copy link
Member Author

phated commented Feb 9, 2016

@segrey --tasks-json already takes a path argument to output to the filesystem. I don't want to overload the flags.

@segrey
Copy link

segrey commented Feb 9, 2016

@phated Oops, sure. Then it can be --depth=top, --depth=full.

@phated
Copy link
Member Author

phated commented Feb 9, 2016

@segrey depth is currently a number and I think the number makes more sense because you can specify the depth you are looking for.

@segrey
Copy link

segrey commented Feb 9, 2016

@phated I'm okay with current behavior (when depth is a numerical flag). IIUC, you'd like to not mix number and string values (e.g. --depth=1 and --depth=non-recursive). FWIW, an alternative option could be to have string values only.

@tylersticka
Copy link

It's not too clear what the parallel and series tasks are. Maybe (series) or (parallel) could come after the top level task name? e.g. build (series)

Hmm... I don't love that idea because I'm not sure how it would scale to series or parallel tasks that follow a series or parallel task. Would it concatenate, saying build (series) (series)?

Here's a simpler idea... restoring the cyan coloring to child tasks, but retaining the gray for the series and parallel headings:

screen shot 2016-02-09 at 2 57 10 pm

@phated
Copy link
Member Author

phated commented Feb 9, 2016

@tylersticka The output for the above doesn't seem correct. "combined" has 3 children - "clean", "parallel" and "series" but no indicator of how those children are executing.

I think the 2 things that need to be tackled are:

  1. parallel and series clutter the task listing but have crucial information on how things are executing - this is the one you are currently mocking up
  2. If a child task is used in another dependency tree, the entire child tree is duplicated in the output - suggestions for solving this include a color or some other indicator of a duplicate

@tylersticka
Copy link

Thanks, @phated !

The output for the above doesn't seem correct. "combined" has 3 children - "clean", "parallel" and "series" but no indicator of how those children are executing.

You're right, there would be a "series" heading at the beginning based on what I was referencing. Your suggestion makes more sense to me now.

If a child task is used in another dependency tree, the entire child tree is duplicated in the output - suggestions for solving this include a color or some other indicator of a duplicate

Can you provide an example for me (or link to a pre-existing one in this thread)? I'm sure you're explaining it accurately but I feel like I'm still not understanding completely.

(And if at any point my mockups are complicating more than illuminating things, just let me know! I promise I won't take offense. 😄 )

@phated
Copy link
Member Author

phated commented Feb 9, 2016

@tylersticka np, I am really happy to have someone with an eye for design helping on these problems. The best place to look for an example is the base issue at gulpjs/gulp#1033 - it shows how the output grows exponentially for some given gulpfiles

@tylersticka
Copy link

@phated Just read through gulpjs/gulp#1033 again. It's a bit difficult for me to parse, but it sounds to me like what we'd like to do is visualize when a task is being called repeatedly within the same task... so if task a3 calls task a1 and a2, but task a2 already calls a1, we would highlight that duplicate.

If that's the case, I feel like coloring the duplicate tasks red would not be too strong. I'm sure my Gulp usage is less complicated than average, but if I accidentally had a task called multiple times I'd certainly want it visualized in a strong way.

An example:

screen shot 2016-02-09 at 4 04 43 pm

I may have misread or only partially-understood the design ask here, so feel free to course-correct me.

@sttk
Copy link
Contributor

sttk commented Feb 10, 2016

@phate Sorry, and thanks about branch flag.

I've created a sample program to hide series/parallel tasks in outputs of --tasks. (But I don't think that hiding series/parallel tasks is better, though.)

> node tasksTest

Tasks (Not hide branch tasks)
├─┬ task0
│ └─┬ <series>
│   ├─┬ <parallel>
│   │ ├─┬ <series>
│   │ │ ├── task2
│   │ │ ├─┬ <palallel>
│   │ │ │ ├── task8
│   │ │ │ └── task9
│   │ │ └── task3
│   │ ├── task4
│   │ └── task6
│   └── task7
└── task1

Tasks (Hide branch tasks)
├─┬ task0
│ └─┬─┬─┬── task2
│   │ │ ├─┬── task8
│   │ │ │ └── task9
│   │ │ └── task3
│   │ ├── task4
│   │ └── task6
│   └── task7
└── task1

@sttk
Copy link
Contributor

sttk commented Feb 27, 2016

I've reflected the addition of branch flag by gulpjs/undertaker#60 and separated compact (non-recursive) option from depth option in my repository:

sttk/gulp-cli#modify_for_tasks_json_norcr

@phated
Copy link
Member Author

phated commented Apr 5, 2016

@sttk is this worth sending as a PR or are you planning to make some more changes?

@sttk
Copy link
Contributor

sttk commented Apr 5, 2016

@phated I have no more changes. I've send the PR about this issue.

@phated phated modified the milestones: 1.3.0, 1.4.0 Apr 23, 2017
@phated phated closed this as completed in 86f5df8 Jun 12, 2017
phated pushed a commit that referenced this issue Dec 21, 2017
phated pushed a commit that referenced this issue Dec 21, 2017
phated pushed a commit that referenced this issue Dec 21, 2017
phated pushed a commit that referenced this issue Dec 21, 2017
phated pushed a commit that referenced this issue Dec 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants