Integrate colorama for coloured output #164

decentral1se · 2019-05-12T09:37:42Z

decentral1se commented

2019-05-12 09:37:42 +00:00

This would be a first step to enable a custom logger for all output that is controlled by the existing logging implementation. This does not cover the raising of ValueErrors or the output from Borg itself.

Just getting this up early to see if we agree on a few things. If using colorama is OK, adding it as a dependency, the custom logger implementation and colours used etc.

I could continue on this PR if we're agreed that this is the right direction or we could merge it as I work onwards to cover the rest of the outputting.

Working on https://projects.torsion.org/witten/borgmatic/issues/125. This would be a first step to enable a custom logger for all output that is controlled by the existing logging implementation. This does not cover the raising of ValueErrors or the output from Borg itself. Just getting this up early to see if we agree on a few things. If using colorama is OK, adding it as a dependency, the custom logger implementation and colours used etc. I could continue on this PR if we're agreed that this is the right direction or we could merge it as I work onwards to cover the rest of the outputting.

witten commented

2019-05-12 23:59:13 +00:00

I think taking a dependency on colorama is fine. It doesn't pull in any other dependencies or require C library compilation. And it has a compatible license. It hasn't seen a release since last year, but I imagine that's because the library is pretty much "done".

I also think that this PR is a good first step towards colored output. It doesn't have to color absolutely everything out of the gate (e.g. Borg output) to be useful. So I do think it'd be better to merge this iteration prior to expanding its scope. First though I do have a few bits of minor feedback I'll include in a review. Thanks!

I think taking a dependency on colorama is fine. It doesn't pull in any other dependencies or require C library compilation. And it has a compatible license. It hasn't seen a release since last year, but I imagine that's because the library is pretty much "done". I also think that this PR is a good first step towards colored output. It doesn't have to color absolutely everything out of the gate (e.g. Borg output) to be useful. So I do think it'd be better to merge this iteration prior to expanding its scope. First though I do have a few bits of minor feedback I'll include in a review. Thanks!

witten reviewed 2019-05-13 00:11:54 +00:00

witten left a comment

Cool!

borgmatic/commands/borgmatic.py Outdated

						
				@ -22,10 +23,10 @@ from borgmatic.commands import hook

				from borgmatic.config import checks, collect, convert, validate

				from borgmatic.signals import configure_signals

				from borgmatic.verbosity import verbosity_to_log_level

				from borgmatic.logger import should_do_markup, get_logger

witten commented

2019-05-13 00:00:33 +00:00

Super minor: I tend to alpha order imports within each group, just to make visual scanning easier. E.g., from borgmatic.logger would go on the line after from borgmatic.config.

Super minor: I tend to alpha order imports within each group, just to make visual scanning easier. E.g., `from borgmatic.logger` would go on the line after `from borgmatic.config`.

decentral1se commented

2019-05-13 11:22:54 +00:00

I'll fix. I'd recommend integrating https://isort.readthedocs.io/en/latest/ so that the tooling mandates and saves you time checking it yourself. Here's a configuration I use myself that plays well with Black: https://git.coop/decentral1se/pypkgtemplate/blob/master/%7B%7Bcookiecutter.package%7D%7D/setup.cfg#L7.

witten commented

2019-05-13 19:46:54 +00:00

Point taken. I filed witten/borgmatic#169 for that.

Point taken. I filed https://projects.torsion.org/witten/borgmatic/issues/169 for that.

borgmatic/commands/borgmatic.py Outdated

						
				@ -170,2 +171,4 @@

				        help='Go through the motions, but do not actually write to any repositories',

				    )

				    common_group.add_argument(

				        '-nc', '--no-colour', dest='no_colour', action='store_true', help='Disable coloured output'

witten commented

2019-05-13 00:02:40 +00:00

May seem silly, but I think I'd prefer American spelling for this. I don't have any good reason except perhaps it's just more common among users: https://en.wikipedia.org/wiki/Comparison_of_American_and_British_English#Demographics

But if you like, you can include flags for both (e.g. --no-color and --no-colour) to be more inclusive!

May seem silly, but I think I'd prefer American spelling for this. I don't have any good reason except perhaps it's just more common among users: https://en.wikipedia.org/wiki/Comparison_of_American_and_British_English#Demographics But if you like, you can include flags for both (e.g. `--no-color` and `--no-colour`) to be more inclusive!

borgmatic/logger.py Outdated

						
				@ -0,0 +18,4 @@

				    if arg in ('yes', 'on', '1', 'true', 1):

				        return True

				    return False

witten commented

2019-05-13 00:05:08 +00:00

It'd be good to have a simple automated test or three for this function, given that it's pure logic.

decentral1se commented

2019-05-13 11:29:44 +00:00

Makes sense! Also, to note, I took this implementation from the Ansible utility, so it is well battle tested for this use case.

borgmatic/logger.py Outdated

						
				@ -0,0 +33,4 @@

				    if py_colors is not None:

				        return to_bool(py_colors)

				    return sys.stdout.isatty() and os.environ.get('TERM') != 'dumb'

witten commented

2019-05-13 00:11:33 +00:00

Nice to have the auto-detection here!

borgmatic/logger.py Outdated

						
				@ -0,0 +64,4 @@

				    '''

				    Give red coloured text.

				    '''

				    return color_text(colorama.Fore.RED, msg)

witten commented

2019-05-13 00:08:14 +00:00

Given that these *_text() functions are only used in one place, I might be inclined to just inline them there. Do not feel strongly though.

Given that these `*_text()` functions are only used in one place, I might be inclined to just inline them there. Do not feel strongly though.

docs/how-to/set-up-backups.md Outdated

						
				@ -182,0 +183,4 @@

				Borgmatic uses [colorama](https://pypi.org/project/colorama/) to produce

				coloured terminal output by default. It can be disable by passing `--no-colour`

				or by setting the environment variable `PY_COLORS=False`.

witten commented

2019-05-13 00:10:23 +00:00

Given that borgmatic is used so often from cron jobs, a reader's first reaction might be: "Wait.. Does that mean I need to specify --no-colour if I want to call borgmatic from a cron job?" To get ahead of that concern, it might be good to state here that the "by default" color won't happen on a non-interactive terminal (like a cron job).

Given that borgmatic is used so often from cron jobs, a reader's first reaction might be: "Wait.. Does that mean I need to specify `--no-colour` if I want to call borgmatic from a cron job?" To get ahead of that concern, it might be good to state here that the "by default" color won't happen on a non-interactive terminal (like a cron job).

decentral1se commented

2019-05-13 11:42:10 +00:00

Thanks for the review. Comments addressed 👍

Thanks for the review. Comments addressed :thumbsup:

witten commented

2019-05-13 19:50:16 +00:00

Awesome, thank you!

witten closed this pull request

2019-05-13 19:50:37 +00:00

decentral1se deleted branch feature/coloured-output-first-step

2019-05-13 20:39:06 +00:00

witten commented

2019-05-13 20:51:52 +00:00

In playing around with this, I found the coloring does not appear to affect critical/error messages that get logged by logging.handle(). So I implemented support for that too: 37362150fe

In playing around with this, I found the coloring does not appear to affect critical/error messages that get logged by `logging.handle()`. So I implemented support for that too: https://projects.torsion.org/witten/borgmatic/commit/37362150fe4a29449298c7b6c690ffee23060899

🎉 1

Sign in to join this conversation.

No reviewers