WIP Replace subprocess calls with a generic run function #109

Closed
floli wants to merge 2 commits from (deleted):run_function into main
8 changed files with 29 additions and 26 deletions
Showing only changes of commit 3e3e411bf4 - Show all commits

View File

@ -117,8 +117,7 @@ def check_archives(
# The check command spews to stdout/stderr even without the verbose flag. Suppress it.
stdout = None if verbosity_flags else open(os.devnull, 'w')
logger.debug(' '.join(full_command))
subprocess.check_call(full_command, stdout=stdout, stderr=subprocess.STDOUT)
run(full_command, stdout=stdout, stderr=subprocess.STDOUT)
if 'extract' in checks:
extract.extract_last_archive_dry_run(repository, lock_wait, local_path, remote_path)

View File

@ -2,9 +2,9 @@ import glob
import itertools
import logging
import os
import subprocess
import tempfile
from borgmatic.run import run
logger = logging.getLogger(__name__)
@ -154,5 +154,4 @@ def create_archive(
+ (('--json',) if json else ())
)
logger.debug(' '.join(full_command))
subprocess.check_call(full_command)
return run(full_command, capture_output = json)
Review

Note that when you run the Black code formatter on this (part of running borgmatic's tox), it will complain about the format on lines like this one. You can always run tox -e black though to have it auto-reformat them for you.

Note that when you run the [Black](https://github.com/ambv/black) code formatter on this (part of running borgmatic's tox), it will complain about the format on lines like this one. You can always run `tox -e black` though to have it auto-reformat them for you.

View File

@ -1,7 +1,7 @@
import logging
import sys
import subprocess
from borgmatic.run import run
logger = logging.getLogger(__name__)
@ -26,7 +26,7 @@ def extract_last_archive_dry_run(repository, lock_wait=None, local_path='borg',
+ verbosity_flags
)
list_output = subprocess.check_output(full_list_command).decode(sys.stdout.encoding)
list_output = run(full_list_command, capture_output = True)
last_archive_name = list_output.strip().split('\n')[-1]
if not last_archive_name:
@ -48,5 +48,4 @@ def extract_last_archive_dry_run(repository, lock_wait=None, local_path='borg',
+ list_flag
)
logger.debug(' '.join(full_extract_command))
subprocess.check_call(full_extract_command)
run(full_extract_command)

View File

@ -1,6 +1,6 @@
import logging
import subprocess
from borgmatic.run import run
logger = logging.getLogger(__name__)
@ -23,7 +23,4 @@ def display_archives_info(
+ (('--json',) if json else ())
)
logger.debug(' '.join(full_command))
output = subprocess.check_output(full_command)
return output.decode() if output is not None else None
return run(full_command, capture_output = json)

View File

@ -1,6 +1,6 @@
import logging
import subprocess
from borgmatic.run import run
logger = logging.getLogger(__name__)
@ -20,7 +20,5 @@ def list_archives(repository, storage_config, local_path='borg', remote_path=Non
+ (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ())
+ (('--json',) if json else ())
)
logger.debug(' '.join(full_command))
output = subprocess.check_output(full_command)
return output.decode() if output is not None else None
run(full_command, capture_output = json)
Review

Needs to return the result of the run() invocation?

Needs to `return` the result of the `run()` invocation?

View File

@ -1,6 +1,6 @@
import logging
import subprocess
from borgmatic.run import run
logger = logging.getLogger(__name__)
@ -53,5 +53,4 @@ def prune_archives(
+ (('--dry-run',) if dry_run else ())
)
logger.debug(' '.join(full_command))
subprocess.check_call(full_command)
run(full_command)

View File

@ -205,7 +205,7 @@ def _run_commands_on_repository(
)
if args.create:
logger.info('{}: Creating archive{}'.format(repository, dry_run_label))
borg_create.create_archive(
output = borg_create.create_archive(
args.dry_run,
repository,
location,
@ -213,6 +213,8 @@ def _run_commands_on_repository(
local_path=local_path,
remote_path=remote_path,
)
if args.json:
json_results.append(json.loads(output))
Review

Would it make sense to do if output: instead of if args.json here? That way, you don't have the args.json-checking logic in two places (1. in create_archive() and, 2. here). Similar elsewhere in this function. Do no feel super strongly.

Would it make sense to do `if output:` instead of `if args.json` here? That way, you don't have the `args.json`-checking logic in two places (1. in `create_archive()` and, 2. here). Similar elsewhere in this function. Do no feel super strongly.
if args.check and checks.repository_enabled_for_checks(repository, consistency):
logger.info('{}: Running consistency checks'.format(repository))
borg_check.check_archives(
@ -225,8 +227,6 @@ def _run_commands_on_repository(
)
if args.json:
json_results.append(json.loads(output))
else:
sys.stdout.write(output)
if args.info:
logger.info('{}: Displaying summary info for archives'.format(repository))
output = borg_info.display_archives_info(
@ -234,8 +234,6 @@ def _run_commands_on_repository(
)
if args.json:
json_results.append(json.loads(output))
else:
sys.stdout.write(output)
def main(): # pragma: no cover

14
borgmatic/run.py Normal file
View File

@ -0,0 +1,14 @@
import logging
import subprocess
logger = logging.getLogger(__name__)
def run(args, capture_output = False, **kwargs):
Review

Would be nice to have a doc string for this function, even though it's pretty self-explanitory.

Also, given that this function is only used within borgmatic/borg/, this file might belong in that directory. (Do not feel strongly.)

Would be nice to have a doc string for this function, even though it's pretty self-explanitory. Also, given that this function is only used within `borgmatic/borg/`, this file might belong in that directory. (Do not feel strongly.)
logger.debug('Executing: ' + ' '.join(args))
Review

Might be a little more idiomatic to do something like: logger.debug('Executing: {}'.format(...))

Might be a little more idiomatic to do something like: `logger.debug('Executing: {}'.format(...))`
if capture_output:
return subprocess.check_output(args, **kwargs).decode()
subprocess.check_call(args, **kwargs)
Review

This function could use some very basic test coverage. Basically: The capture output case, and the no capture output case. That'd require a little bit of mocking.

This function could use some very basic test coverage. Basically: The capture output case, and the no capture output case. That'd require a little bit of mocking.