Coloured log output #176

MinchinWeb · 2017-12-09T19:30:34Z

At least on Windows click.echo_through_pager() seems to eat the ANSI colour codes. This PR "fixes" that by allowing the pager to be bypassed and use the "regular" click.echo() (which displays colour codes fine). It does this by adding a --pager/--no-pager option to the log sub-command, and a corresponding options.log_pager configuration setting.

It also makes a couple other small changes:

requires/includes PR Fix setup.py #175 (I can't install watson otherwise...)
changes the formatting slightly of the log's daily heading to better match other output
removes an extra space that is displayed several places is a frame has no corresponding tags.

An issue has been submitted upstream to click (see pallets/click#901) but this seems like a practical work-around.

Also, the main image on the first page of the documentation should be updated. It purportedly demonstrates the report sub-command, but the output looks like it's actually the log sub-command.

Thirdly, is there a comprehensive list of configuration file settings somewhere in the documentation?

Lastly, thanks for making watson; I'm super excited to have discovered it!

SpotlightKid · 2017-12-09T19:33:19Z

Thirdly, is there a comprehensive list of configuration file settings somewhere in the documentation?

https://tailordev.github.io/Watson/user-guide/configuration/

SpotlightKid · 2017-12-09T19:38:17Z

changes the formatting slightly of the log's daily heading to better match other output
removes an extra space that is displayed several places is a frame has no corresponding tags.

Can you please put these into a separate PR (they can go together)?

SpotlightKid · 2017-12-09T19:41:16Z

watson/cli.py

@@ -833,8 +835,8 @@ def remove(watson, id, force):

    if not force:
        click.confirm(
-            "You are about to remove frame "
-            "{project}{tags_spacer}{tags} from {start} to {stop}, continue?".format(
+            'You are about to remove frame {project}{tags_spacer}{tags} from '


Thanks for your effort in unifying single/double quotes, but for messages shown to the user, which are (in theory) translatable, we prefer double quotes. This usage might not be consistent in the existing code, but for new code please adhere to this convention.

Fixed in 903e75f (part of #179)

SpotlightKid · 2017-12-09T19:48:01Z

Also, the main image on the first page of the documentation should be updated.

See #177.

MinchinWeb · 2017-12-09T21:57:11Z

Ok, the other changes have been split off into separate PR's, and everything here just has to do with making the pager on the log output option. Documentation has also been added.

SpotlightKid · 2017-12-10T21:03:44Z

I see that the output of the report command is currently not sent through a pager. I think we should do the same there as for the log command and use a config option name that fits for both, e.g. use_pager.

SpotlightKid · 2017-12-10T21:07:18Z

docs/user-guide/commands.md also needs to be updated by running scripts/gen-cli-docs.py. You should also update the docstrings of the log and report functions and mention the new option.

SpotlightKid

Also please rebase on master.

SpotlightKid · 2017-12-10T20:57:34Z

watson/cli.py

@@ -529,9 +529,11 @@ def report(watson, current, from_, to, projects,
              "times")
 @click.option('-j', '--json', 'format_json', is_flag=True,
              help="Format the log in JSON instead of plain text")
+@click.option('--pager/--no-pager', 'pager', default=None,


There should be a short form of the option too. I'm open to suggestions for the letters to use.

-v/-V (for 'view' maybe'?) it doesn't seem to be used elsewhere.

-v is often used to mean verbose or version, so I don't think it's an ideal choice. How about -l/-L (mnemonic less, but it's rather arbitrary ;))?

SpotlightKid · 2017-12-10T20:59:39Z

watson/cli.py

-    click.echo_via_pager('\n'.join(lines))
+    if pager or (pager is None and
+                 watson.config.getboolean('options', 'log_pager',
+                                          default=True)):


You don't need the default keyword arg name here. Saves you from needing another line break.

I've made the removal like you suggested, but it does change the default behaviour to now bypass the pager. I've added a note to the changelog to try and make this obvious.

Sorry, I didn't make my self clear enough. I only meant:

if pager or (pager is None and watson.config.getboolean('options', 'log_pager', True)):

MinchinWeb · 2017-12-11T17:36:43Z

Changes have been rebased onto master (and force-pushed here).

Pager selective support has been added to the report command. As a by-product of this, all the lines have to be gathered before they're printed, so this make make the command feel slower to some users.

The configuration option for both has been consolidated to options.pager.

Doc strings for both log and report have been updated. Documentation has also been updated using the script.

k4nar · 2017-12-11T17:43:01Z

watson/cli.py

+        click.echo_via_pager('\n'.join(lines))
+    else:
+        click.echo('\n'.join(lines))
+


I think we should avoid buffering all the lines when in paging mode. The way to do it is not obvious, but maybe something like that would work:

lines = [] if pager or (...): def _print(line): lines.append(line) else: def _print(line): click.echo(line) ... _print('{project} - {time}'.format(...)) ... if pager or (...): click.echo('\n'.join(lines)

Done in 046bf4e

includes documentation updates

`log` and `report` where waiting to gather all lines before deciding what to do with them (either pushing them to the terminal directly or through a pager). This change allows the lines to be gathered to the pager, or printed at once to the terminal, as desired.

MinchinWeb · 2017-12-12T00:04:23Z

Lines are now printed in "real time" if printing is direct to the terminal, rather than waiting until end.

SpotlightKid

The comments on the 'reportfunction likewise apply to thelog` function.

SpotlightKid · 2017-12-12T12:19:26Z

docs/user-guide/commands.md

@@ -127,6 +127,9 @@ You can also use special shortcut options for easier timespan control:
 and `--year`, `--month` and `--week` to the current year, month or week
 respectively.

+If you are outputting to the terminal, you can selectively enable a pager
+through the `--pager`` option.


Here's one backtick too many.

SpotlightKid · 2017-12-12T12:20:05Z

docs/user-guide/commands.md

@@ -174,6 +177,7 @@ Flag | Help
 `-p, --project TEXT` | Logs activity only for the given project. You can add other projects by using this option several times.
 `-T, --tag TEXT` | Logs activity only for frames containing the given tag. You can add several tags by using this option multiple times
 `-j, --json` | Format the log in JSON instead of plain text
+`-v, --pager / -V, --no-pager` | (Don't) view output through a pager.


Like I said, I'd rather use another letter for the short option than "v".

SpotlightKid · 2017-12-12T12:20:19Z

docs/user-guide/commands.md

@@ -329,6 +333,9 @@ You can limit the report to a project or a tag using the `--project` and
 `--tag` options. They can be specified several times each to add multiple
 projects or tags to the report.

+If you are outputting to the terminal, you can selectively enable a pager
+through the `--pager`` option.


Here's one backtick too many.

SpotlightKid · 2017-12-12T12:21:52Z

watson/cli.py

@@ -374,6 +376,9 @@ def report(watson, current, from_, to, projects,
    `--tag` options. They can be specified several times each to add multiple
    projects or tags to the report.

+    If you are outputting to the terminal, you can selectively enable a pager
+    through the `--pager`` option.


Here's the source of the extra backtick.

SpotlightKid · 2017-12-12T12:23:15Z

watson/cli.py

+    lines = []
+    # use the pager, or print directly to the terminal
+    if pager or (pager is None and
+            watson.config.getboolean('options', 'pager')):


Should be watson.config.getboolean('options', 'pager', True), no?

Also, I'd line up this line with the second pager above.

Adding 'True' maintains the current default (at least for log) of running output through the pager.

You had me remove this earlier because it was bumping it onto a second line, but the indentation is different now, so that's no longer an issue.

Both fixed!

What I was trying to say all the time is that you don't need the argument name, i.e default=, not the argument value itself.

Aaah, that makes more sense. Updated as recommended!

SpotlightKid · 2017-12-12T12:24:44Z

watson/cli.py

-            style('date', '{:ddd DD MMMM YYYY}'.format(
-                arrow.get(report['timespan']['from'])
+        use_pager = False
+        def _print(line):


There should be a blank line before inline function definitions (see also Travis log).

SpotlightKid · 2017-12-12T12:30:21Z

watson/cli.py

+        style('date', '{:ddd DD MMMM YYYY}'.format(
+            arrow.get(report['timespan']['to'])
+        ))
+        ))


Line should be on the same indentation level as _print.

SpotlightKid · 2017-12-12T12:37:07Z

Regardless of this PR, I really think we should move the logging / reporting functionality into a separate command line script at some point.

MinchinWeb · 2017-12-12T16:40:26Z

All changes as requested have been submitted.

Moving logging/report to a separate module is beyond what I set out to do here, so I'll leave it for a separate PR/refactoring effort.

SpotlightKid · 2017-12-13T22:50:44Z

@MinchinWeb: Thanks for hanging in there. Great work!

MinchinWeb · 2017-12-14T02:40:10Z

Thanks! Glad I can do my part to make the world a little nicer!

willdurand requested a review from SpotlightKid December 9, 2017 19:30

SpotlightKid reviewed Dec 9, 2017

View reviewed changes

SpotlightKid mentioned this pull request Dec 9, 2017

Screenshot on documentation overview page is outdated #177

Open

This was referenced Dec 9, 2017

Standarize formatting of log's daily header line #178

Closed

Fix CLI output spacing if a frame has not tags #179

Merged

MinchinWeb force-pushed the coloured-log-output branch from 70f1245 to c9ae6b6 Compare December 9, 2017 21:53

SpotlightKid suggested changes Dec 10, 2017

View reviewed changes

MinchinWeb force-pushed the coloured-log-output branch from c9ae6b6 to ab7f3ed Compare December 11, 2017 17:36

k4nar reviewed Dec 11, 2017

View reviewed changes

MinchinWeb added 3 commits December 11, 2017 11:16

make log output through pager optional

f6c4bb7

includes documentation updates

Add pager option to report command

4553cea

MinchinWeb force-pushed the coloured-log-output branch from ab7f3ed to 046bf4e Compare December 12, 2017 00:03

SpotlightKid suggested changes Dec 12, 2017

View reviewed changes

MinchinWeb force-pushed the coloured-log-output branch from dd09f6f to e6aff65 Compare December 12, 2017 16:34

Fixes as per code review

a713ffa

MinchinWeb force-pushed the coloured-log-output branch from e6aff65 to a713ffa Compare December 13, 2017 20:28

SpotlightKid approved these changes Dec 13, 2017

View reviewed changes

SpotlightKid merged commit 6257e78 into jazzband:master Dec 13, 2017

MinchinWeb deleted the coloured-log-output branch December 14, 2017 00:03

SpotlightKid added the improvement label Dec 21, 2017

Coloured log output #176

Coloured log output #176

Conversation

MinchinWeb commented Dec 9, 2017

SpotlightKid commented Dec 9, 2017

SpotlightKid commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpotlightKid commented Dec 9, 2017

MinchinWeb commented Dec 9, 2017

SpotlightKid commented Dec 10, 2017

SpotlightKid commented Dec 10, 2017

SpotlightKid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinchinWeb commented Dec 11, 2017

k4nar Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinchinWeb commented Dec 12, 2017

SpotlightKid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpotlightKid Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpotlightKid commented Dec 12, 2017

MinchinWeb commented Dec 12, 2017

SpotlightKid commented Dec 13, 2017

MinchinWeb commented Dec 14, 2017

k4nar Dec 11, 2017 •

edited

Loading

SpotlightKid Dec 12, 2017 •

edited

Loading