From 5c8de2f3d521465354b0514e0bac135caef9eea5 Mon Sep 17 00:00:00 2001 From: Craig de Stigter Date: Thu, 7 Oct 2021 09:26:19 +1300 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Robert Coup --- kart/json_diff_writers.py | 18 ++++++++++++++---- kart/show.py | 2 +- tests/test_diff.py | 4 ++-- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/kart/json_diff_writers.py b/kart/json_diff_writers.py index 202ca229b..b56972cbf 100644 --- a/kart/json_diff_writers.py +++ b/kart/json_diff_writers.py @@ -1,5 +1,6 @@ from datetime import datetime, timezone, timedelta import json +import logging from pathlib import Path import click @@ -11,6 +12,8 @@ from .output_util import dump_json_output, resolve_output_path from .timestamps import datetime_to_iso8601_utc, timedelta_to_iso8601_tz +L = logging.getLogger(__name__) + class JsonDiffWriter(BaseDiffWriter): """ @@ -265,13 +268,20 @@ def _check_output_path(cls, repo, output_path): if isinstance(output_path, Path): if output_path.is_file(): raise click.BadParameter( - "Output path should be a directory for geojson format.", - param_hint="--output", + "Output path should be a directory for GeoJSON format.", + param_hint="--output-path", ) if not output_path.exists(): output_path.mkdir() else: - for p in output_path.glob("*.geojson"): + geojson_paths = list(output_path.glob("*.geojson")) + if geojson_paths: + L.debug( + "Cleaning %d *.geojson files from %s ...", + len(geojson_paths), + output_path, + ) + for p in geojson_paths: p.unlink() else: output_path = "-" @@ -282,7 +292,7 @@ def write_diff(self): if len(self.all_ds_paths) > 1 and not isinstance(self.output_path, Path): raise click.BadParameter( - "Need to specify a directory for geojson with >1 dataset", + "Need to specify a directory via --output-path for GeoJSON with more than one dataset", param_hint="--output", ) for ds_path in self.all_ds_paths: diff --git a/kart/show.py b/kart/show.py index 983c28e0b..516a39148 100644 --- a/kart/show.py +++ b/kart/show.py @@ -10,7 +10,7 @@ @click.option( "--output-format", "-o", - # note: geojson was removed from here because it doesn't work with multiple datasets. + # note: geojson isn't here because it doesn't work cleanly with multiple datasets. type=click.Choice(["text", "json", "quiet", "feature-count", "html", "json-lines"]), default="text", help=( diff --git a/tests/test_diff.py b/tests/test_diff.py index 83f2ae0d5..fdb941944 100644 --- a/tests/test_diff.py +++ b/tests/test_diff.py @@ -1759,7 +1759,7 @@ def test_diff_geojson_usage(data_archive, cli_runner, tmp_path): assert r.exit_code == 2, r.stderr assert ( r.stderr.splitlines()[-1] - == "Error: Invalid value for --output: Need to specify a directory for geojson with >1 dataset" + == "Error: Invalid value for --output: Need to specify a directory via --output-path for GeoJSON with more than one dataset" ) # Can't specify an (existing) regular file either @@ -1777,5 +1777,5 @@ def test_diff_geojson_usage(data_archive, cli_runner, tmp_path): assert r.exit_code == 2, r.stderr assert ( r.stderr.splitlines()[-1] - == "Error: Invalid value for --output: Output path should be a directory for geojson format." + == "Error: Invalid value for --output-path: Output path should be a directory for GeoJSON format." )