Merge pull request #6088 from nilsvu/cli

Improve some CLI errors and docs

docs/DevGuide/PythonBindings.md

@@ -187,11 +187,12 @@ See \ref spectre_using_python "Using SpECTRE's Python"
   subdirectory structures. Passing many files to a script is easy for the user
   by using a glob (note: don't take the glob as a string argument, take the
   expanded list of files directly using `click.argument(..., nargs=-1,
-  type=click.Path(...))`).
+  type=click.Path(...), required=True)`).
+  Note that Click recommends to avoid `required=True` here but to [gracefully
+  degrade to a noop](https://click.palletsprojects.com/en/8.1.x/arguments/#variadic-arguments)
+  instead, but that's confusing for the user.
 - Never overwrite or delete files without prompting the user or asking them to
   run with `--force`.
-- When the input to a script is empty, [gracefully degrade to a
-  noop](https://click.palletsprojects.com/en/8.1.x/arguments/#variadic-arguments).
 - When the user did not specify an option, print possible values for it and
   return instead of raising an exception. For example, print the subfile names
   in an H5 file if no subfile name was specified. This allows the user to make

src/IO/H5/Python/CombineH5.py

@@ -9,6 +9,7 @@
 
 import spectre.IO.H5 as spectre_h5
 from spectre.IO.H5.CombineH5Dat import combine_h5_dat_command
+from spectre.support.CliExceptions import RequiredChoiceError
 
 
 @click.group(name="combine-h5")
@@ -25,6 +26,7 @@ def combine_h5_command():
     "h5files",
     type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
     nargs=-1,
+    required=True,
 )
 @click.option(
     "--subfile-name",
@@ -66,17 +68,16 @@ def combine_h5_vol_command(h5files, subfile_name, output, check_src):
     time steps (e.g. from multiple segments of a simulation). All input H5 files
     must contain the same set of observation IDs.
     """
-    # CLI scripts should be noops when input is empty
-    if not h5files:
-        return
-
     # Print available subfile names and exit
     if not subfile_name:
         spectre_file = spectre_h5.H5File(h5files[0], "r")
-        import rich.columns
-
-        rich.print(rich.columns.Columns(spectre_file.all_vol_files()))
-        return
+        raise RequiredChoiceError(
+            (
+                "Specify '--subfile-name' / '-d' to select a"
+                " subfile containing volume data."
+            ),
+            choices=spectre_file.all_vol_files(),
+        )
 
     if not output.endswith(".h5"):
         output += ".h5"

src/IO/H5/Python/CombineH5Dat.py

@@ -26,10 +26,6 @@ def combine_h5_dat(h5files, output, force):
       output: Output filename. An extension '.h5' will be added if not present.
       force: If specified, overwrite output file if it already exists
     """
-
-    if len(h5files) == 0:
-        return
-
     # Copy first input file to output file
     if not output.endswith(".h5"):
         output += ".h5"
@@ -72,6 +68,7 @@ def combine_h5_dat(h5files, output, force):
         readable=True,
     ),
     nargs=-1,
+    required=True,
 )
 @click.option(
     "--output",

src/IO/H5/Python/DeleteSubfiles.py

@@ -20,6 +20,7 @@
     "h5files",
     type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
     nargs=-1,
+    required=True,
 )
 @click.option(
     "--subfile",

src/Visualization/Python/GenerateXdmf.py

@@ -20,6 +20,7 @@
 import numpy as np
 import rich
 
+from spectre.support.CliExceptions import RequiredChoiceError
 from spectre.Visualization.ReadH5 import available_subfiles
 
 logger = logging.getLogger(__name__)
@@ -303,23 +304,25 @@ def generate_xdmf(
       coordinates: Optional. Name of coordinates dataset. Default:
         "InertialCoordinates".
     """
-    # CLI scripts should be noops when input is empty
-    if not h5files:
-        return
-
     h5files = [(h5py.File(filename, "r"), filename) for filename in h5files]
 
     if not subfile_name:
-        import rich.columns
-
         subfiles = available_subfiles(
             (h5file for h5file, _ in h5files), extension=".vol"
         )
         if len(subfiles) == 1:
             subfile_name = subfiles[0]
+            logger.info(
+                f"Selected subfile {subfile_name} (the only available one)."
+            )
         else:
-            rich.print(rich.columns.Columns(subfiles))
-            return
+            raise RequiredChoiceError(
+                (
+                    "Specify '--subfile-name' / '-d' to select a"
+                    " subfile containing volume data."
+                ),
+                choices=subfiles,
+            )
 
     if not subfile_name.endswith(".vol"):
         subfile_name += ".vol"
@@ -445,6 +448,7 @@ def generate_xdmf(
     "h5files",
     type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
     nargs=-1,
+    required=True,
 )
 @click.option(
     "--output",

src/Visualization/Python/OpenVolfiles.py

@@ -11,8 +11,11 @@
 import rich
 
 import spectre.IO.H5 as spectre_h5
+from spectre.support.CliExceptions import RequiredChoiceError
 from spectre.Visualization.ReadH5 import list_observations, select_observation
 
+logger = logging.getLogger(__name__)
+
 
 def open_volfiles(
     h5_files: Iterable[str], subfile_name: str, obs_id: Optional[int] = None
@@ -87,6 +90,7 @@ def decorator(f):
         @click.argument(
             "h5_files",
             nargs=-1,
+            required=True,
             type=click.Path(
                 exists=True, file_okay=True, dir_okay=False, readable=True
             ),
@@ -95,7 +99,8 @@ def decorator(f):
             "--subfile-name",
             "-d",
             help=(
-                "Name of subfile within h5 file containing volume data to plot."
+                "Name of subfile within H5 file containing volume data to plot."
+                " Optional if the H5 files have only one subfile."
             ),
         )
         @click.option(
@@ -114,7 +119,8 @@ def decorator(f):
                 "in the volume data file, such as 'Shift_x'. "
                 "Also accepts glob patterns like 'Shift_*'."
             )
-            + (" Can be specified multiple times." if multiple_vars else ""),
+            + (" Can be specified multiple times." if multiple_vars else "")
+            + ("  [required]" if vars_required else ""),
         )
         @click.option(
             "--list-observations",
@@ -154,21 +160,24 @@ def command(
             time,
             **kwargs,
         ):
-            # Script should be a noop if input files are empty
-            if not h5_files:
-                return
-
             # Print available subfile names and exit
             if not subfile_name:
-                import rich.columns
-
                 with spectre_h5.H5File(h5_files[0], "r") as open_h5_file:
                     available_subfiles = open_h5_file.all_vol_files()
                 if len(available_subfiles) == 1:
                     subfile_name = available_subfiles[0]
+                    logger.info(
+                        f"Selected subfile {subfile_name}"
+                        " (the only available one)."
+                    )
                 else:
-                    rich.print(rich.columns.Columns(available_subfiles))
-                    return
+                    raise RequiredChoiceError(
+                        (
+                            "Specify '--subfile-name' / '-d' to select a"
+                            " subfile containing volume data."
+                        ),
+                        choices=available_subfiles,
+                    )
 
             # Print available observations/times and exit
             if list_times:
@@ -189,6 +198,10 @@ def command(
                     )
                     if len(all_obs_ids) == 1:
                         obs_id, obs_time = all_obs_ids[0], all_obs_times[0]
+                        logger.info(
+                            f"Selected observation at t = {obs_time:g}"
+                            " (the only available one)."
+                        )
                     else:
                         raise click.UsageError(
                             "Specify '--step' or '--time' to select an"
@@ -210,21 +223,26 @@ def command(
                     obs_id or volfile.list_observation_ids()[0]
                 )
                 break
-            if list_vars or (vars_required and not vars_patterns):
+            if list_vars:
                 import rich.columns
 
                 rich.print(rich.columns.Columns(all_vars))
                 return
+            elif vars_required and not vars_patterns:
+                raise RequiredChoiceError(
+                    "Specify '--var' / '-y' to select a variable to plot.",
+                    choices=all_vars,
+                )
             # Expand globs in vars
             vars = []
             if not multiple_vars:
                 vars_patterns = [vars_patterns] if vars_patterns else []
             for var_pattern in vars_patterns:
                 matched_vars = fnmatch.filter(all_vars, var_pattern)
                 if not matched_vars:
-                    raise click.UsageError(
-                        f"The pattern '{var_pattern}' matches no variables. "
-                        f"Available variables are: {all_vars}"
+                    raise RequiredChoiceError(
+                        f"The pattern '{var_pattern}' matches no variables.",
+                        choices=all_vars,
                     )
                 vars.extend(matched_vars)
             # Remove duplicates. Ordering is lost, but that's not important here.

src/Visualization/Python/PlotAlongLine.py

@@ -48,13 +48,16 @@ def points_on_line(
 
 @click.command(name="along-line")
 @open_volfiles_command(obs_id_required=False, multiple_vars=True)
+# Line options
+# These aren't marked "required" so the user can omit them when using options
+# like '--list-vars'.
 @click.option(
     "--line-start",
     "-A",
     callback=parse_point,
     help=(
         "Coordinates of the start of the line through the volume data. "
-        "Specify as comma-separated list, e.g. '0,0,0'."
+        "Specify as comma-separated list, e.g. '0,0,0'.  [required]"
     ),
 )
 @click.option(
@@ -63,7 +66,7 @@ def points_on_line(
     callback=parse_point,
     help=(
         "Coordinates of the end of the line through the volume data. "
-        "Specify as comma-separated list, e.g. '1,0,0'."
+        "Specify as comma-separated list, e.g. '1,0,0'.  [required]"
     ),
 )
 @click.option(

src/Visualization/Python/PlotCce.py

@@ -11,6 +11,7 @@
 import matplotlib.pyplot as plt
 import pandas as pd
 
+from spectre.support.CliExceptions import RequiredChoiceError
 from spectre.Visualization.Plot import (
     apply_stylesheet_command,
     show_or_save_plot_command,
@@ -205,10 +206,12 @@ def plot_cce_command(
         )
         cce_subfile = h5file.get(cce_subfile_name)
         if cce_subfile is None:
-            raise click.UsageError(
-                f"Could not find Cce subfile {cce_subfile} in H5 file"
-                f" {h5_filename}. Available subfiles"
-                f" are:\n{available_subfiles(h5file, extension='cce')}"
+            raise RequiredChoiceError(
+                (
+                    f"Could not find Cce subfile {cce_subfile} in H5 file"
+                    f" {h5_filename}."
+                ),
+                choices=cce_subfiles,
             )
 
         suffix = ".dat" if backward_cce_group is not None else ""

src/Visualization/Python/PlotControlSystem.py

@@ -12,6 +12,7 @@
 import numpy as np
 import pandas as pd
 
+from spectre.support.CliExceptions import RequiredChoiceError
 from spectre.Visualization.Plot import (
     apply_stylesheet_command,
     show_or_save_plot_command,
@@ -26,6 +27,7 @@
     "reduction_files",
     type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
     nargs=-1,
+    required=True,
 )
 @click.option(
     "--with-shape/--without-shape",
@@ -105,10 +107,12 @@ def check_control_system_dir(h5_filename: str):
         h5file = h5py.File(h5_filename, "r")
         control_system_dir = h5file.get("ControlSystems")
         if control_system_dir is None:
-            raise click.UsageError(
-                "Unable to open group 'ControlSystems' from h5 file"
-                f" {h5_filename}. Available subfiles are:\n"
-                f" {available_subfiles(h5file, extension='.dat')}"
+            raise RequiredChoiceError(
+                (
+                    "Unable to open group 'ControlSystems' from h5 file"
+                    f" {h5_filename}."
+                ),
+                choices=available_subfiles(h5file, extension=".dat"),
             )
 
         # No size control
@@ -128,10 +132,12 @@ def check_control_system_file(
         )
         subfile = h5file.get(subfile_path)
         if subfile_path is None:
-            raise click.UsageError(
-                f"Unable to open control system file '{subfile_path}'"
-                f" from h5 file {h5_filename}. Available subfiles are:\n"
-                f" {available_subfiles(h5file, extension='.dat')}"
+            raise RequiredChoiceError(
+                (
+                    f"Unable to open control system subfile '{subfile_path}'"
+                    f" from h5 file {h5_filename}."
+                ),
+                choices=available_subfiles(h5file, extension=".dat"),
             )
 
         return to_dataframe(subfile).set_index("Time")