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

Colorize --list, --list-all and --summary output #874

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Conversation

pd93
Copy link
Member

@pd93 pd93 commented Sep 29, 2022

@andreynering @orenmazor This seemed like low-hanging fruit, so I've done a quick first draft (see the associated branch and screenshots:

task --list-all:

image

task foo --summary:

image

Let me know what you think of the colours - I'm happy to change them. I went with:

  • green for tasks as this matches the green when running a task
  • cyan for descriptions as it complements the green (and the Task logo)
  • yellow for commands

Could we do something more interesting with namespaces? Each layer of the namespace be a different colour? This might be too noisy... Or maybe just a different colour for the colons?


Sidenote: I noticed that logger.Outf automatically adds a \n to the input string. I'm not a fan of this behaviour as it gives less control to the caller. The new logger.FOutf function does not do this to mimic the behaviour of the standard fmt.Printf functions and this allows me to use multiple colours on the same line.

I've left the existing logger.Outf functionality as-is for now, but I think we should consider changing this in the future.

@andreynering
Copy link
Member

Hi @pd93, thanks for taking this one!

I liked the color choices in overall, but I think longer texts like descriptions are better as white. I also tried putting * in yellow and liked the result. What do you think?

Screen Shot 2022-09-29 at 21 49 36

help.go Outdated
e.Logger.FOutf(w, logger.Green, task.Name())
fmt.Fprintf(w, ": \t")
e.Logger.FOutf(w, logger.Cyan, task.Desc)
fmt.Fprintf(w, "\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency you could use e.Logger.FOutf for all calls, but using logger.Default for the white texts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did give this a go, but it resulted in this weird spacing issue and I couldn't figure out what was going on (see red mark in screenshot below).

diff --git a/help.go b/help.go
index 3457683..4914347 100644
--- a/help.go
+++ b/help.go
@@ -44,11 +44,11 @@ func (e *Executor) printTasks(listAll bool) {
        // Format in tab-separated columns with a tab stop of 8.
        w := tabwriter.NewWriter(e.Stdout, 0, 8, 6, ' ', 0)
        for _, task := range tasks {
-               fmt.Fprintf(w, "* ")
+               e.Logger.FOutf(w, logger.Default, "* ")
                e.Logger.FOutf(w, logger.Green, task.Name())
-               fmt.Fprintf(w, ": \t")
+               e.Logger.FOutf(w, logger.Default, ": \t")
                e.Logger.FOutf(w, logger.Cyan, task.Desc)
-               fmt.Fprintf(w, "\n")
+               e.Logger.FOutf(w, logger.Default, "\n")
        }
        w.Flush()
 }

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's fine to keep as is then...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be nicer to be consistent. I'll take another look at this tomorrow and see if I can figure out what's going on.

Copy link
Member Author

@pd93 pd93 Oct 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of time to look at this today and worked out what's going on. The first thing to note is that text/tabwriter doesn't care about escape sequences. This means that when you add color escape sequences, you can get some strange behaviour.

To make debugging easier, I replaced the tab writer with a regular bytes.buffer and printed the output. This is what we were getting (I've reformatted this slightly so its easier to tell what's going on):

"       \x1b[33m* \x1b[0m\x1b[32m <task_1> \x1b[0m\x1b[0m: \t <task 1 desc>"
"\x1b[0m\x1b[33m* \x1b[0m\x1b[32m <task_2> \x1b[0m\x1b[0m: \t <task 2 desc>"
...
"\x1b[0m\x1b[33m* \x1b[0m\x1b[32m <task_n> \x1b[0m\x1b[0m: \t <task n desc>"
"\x1b[0m"

As you can see, because each color print function wraps the text sent to the writer, when you send a \n to it, the color reset escape sequence will be printed on the beginning of the next line rather than at the end of the line being sent to the writer. This means that the tabwriter package will see the next line (and all subsequent lines) as longer than the first line and this leads to oddities with the columns.

As there is no way to change this behaviour in text/tabwriter (I've tried using tabwriter.StripEscape to no effect). I propose that we consistently use e.Logger.FOutf for all output except the final newline which will use fmt.Fprint. This will result in the following:

"\x1b[33m* \x1b[0m\x1b[32m <task_1>           \x1b[0m\x1b[0m: \t <task_1_desc> \x1b[0m"
"\x1b[33m* \x1b[0m\x1b[32m <task_2>           \x1b[0m\x1b[0m: \t <task_2_desc> \x1b[0m"
...
"\x1b[33m* \x1b[0m\x1b[32m <task_n>           \x1b[0m\x1b[0m: \t <task_n_desc> \x1b[0m"

Because the escape sequences are consistent on every line, text/tabwriter will now deal with it correctly.

Updated in c0f8eb0

@pd93
Copy link
Member Author

pd93 commented Sep 30, 2022

I liked the color choices in overall, but I think longer texts like descriptions are better as white. I also tried putting * in yellow and liked the result. What do you think?

Happy to keep the text descriptions white, that makes sense. I assume we should change the description in the --summary to white too? Updated in 2df6908

@andreynering
Copy link
Member

I assume we should change the description in the --summary to white too?

IMO, yes.

@pd93 pd93 marked this pull request as ready for review October 1, 2022 16:55
@andreynering andreynering added the type: enhancement A change to an existing feature or functionality. label Oct 7, 2022
Copy link
Member

@andreynering andreynering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pd93 This needs conflicts to be resolved, a CHANGELOG entry, and maybe squash commits into a single one.

After that, I authorize you to merge this PR yourself if you want. 🙂

feat: add coloured output to --list and --list-all

feat: add coloured output to --summary

chore: update colors

refactor: better consistency in writer calls in printTasks

feat: subtasks are printed green in --summary
@pd93 pd93 merged commit d2061ec into master Oct 7, 2022
@pd93 pd93 deleted the 845-colorize-output branch October 7, 2022 10:05
maxpushka pushed a commit to maxpushka/task that referenced this pull request Oct 10, 2022
@pd93 pd93 linked an issue Oct 21, 2022 that may be closed by this pull request
@pd93 pd93 removed a link to an issue Oct 21, 2022
@pd93 pd93 mentioned this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A change to an existing feature or functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make "--list" and "--list-all" output colored
2 participants