Apply suggestions from code review

Co-authored-by: Robert Coup <[email protected]>
koordinates · Oct 6, 2021 · 5c8de2f · 5c8de2f
1 parent b087808
commit 5c8de2f
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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."
         )