Addressed review comments

.github/workflows/ci.yml

@@ -110,8 +110,8 @@ jobs:
           GAMS_LICENSE: ${{ secrets.GAMS_LICENSE }}
         if: ${{ env.GAMS_LICENSE == '' }}
         working-directory: xl2times
-        # Run without --dd flag GAMS license secret doesn't exist to see if we're just missing a GAMS license.
-        # This way we still run CSV-only regression tests without GAMS - useful for testing in forks before creating PRs.
+        # Run without --dd flag if GAMS license secret doesn't exist.
+        # Useful for testing for (CSV) regressions in forks before creating PRs.
         run: |
           source .venv/bin/activate
           export PATH=$PATH:$GITHUB_WORKSPACE/GAMS/gams44.1_linux_x64_64_sfx
@@ -120,7 +120,6 @@ jobs:
               --verbose \
               | tee out.txt; \
           echo ${PIPESTATUS[0]} > retcode.txt)
-          echo 'Note: Pipeline will fail in final step due to missing GAMS license'
 
       - name: Print summary
         working-directory: xl2times

.gitignore

@@ -16,3 +16,4 @@ speedscope.json
 .venv*/
 benchmarks/
 .idea/
+.python-version

README.md

@@ -53,7 +53,7 @@ If you want to skip these pre-commit steps for a particular commit, if for insta
 git commit --no-verify
 ```
 
-### Publishing the Tool
+## Publishing the Tool
 
 To publish a new version of the tool on PyPI, update the version number in `pyproject.toml`, and then run:
 ```bash
@@ -87,6 +87,7 @@ Note that this assumes you have access to all these repositories (some are priva
 you'll have to request access) - if not, comment out the inaccessible benchmarks from `benchmakrs.yml` before running.
 
 ```bash
+mkdir benchmarks
 # Get VEDA example models and reference DD files
 # XLSX files are in private repo for licensing reasons, please request access or replace with your own licensed VEDA example files.
 git clone git@github.com:olejandro/demos-xlsx.git benchmarks/xlsx/
@@ -99,7 +100,7 @@ git clone git@github.com:esma-cgep/tim-gams.git benchmarks/dd/Ireland
 Then to run the benchmarks:
 ```bash
 # Run a only a single benchmark by name (see benchmarks.yml for name list)
-python utils/run_benchmarks.py benchmarks.yml --verbose --run DemoS_001-all
+python utils/run_benchmarks.py benchmarks.yml --verbose --run DemoS_001-all | tee out.txt
 
 # Run all benchmarks (without GAMS run, just comparing CSV data)
 python utils/run_benchmarks.py benchmarks.yml --verbose | tee out.txt

pyproject.toml

@@ -3,7 +3,7 @@ requires = ["setuptools>=61.0.0", "wheel"]
 build-backend = "setuptools.build_meta"
 
 [tool.setuptools]
-packages = ["xl2times"]
+packages = ["xl2times", "utils"]
 
 [project]
 name = "xl2times"

utils/run_benchmarks.py

@@ -150,8 +150,6 @@ def run_benchmark(
                 stdout=subprocess.PIPE,
                 stderr=subprocess.STDOUT,
                 text=True,
-                # windows needs this for subprocess to inherit venv when calling python directly:
-                shell=True if os.name == "nt" else False,
             )
             if res.returncode != 0:
                 # Remove partial outputs
@@ -160,7 +158,7 @@ def run_benchmark(
                 print(f"ERROR: dd_to_csv failed on {benchmark['name']}")
                 sys.exit(5)
         else:
-            # subprocesses use too much RAM in windows, just use function call in current process instead
+            # If debug option is set, run as a function call to allow stepping with a debugger.
             from utils.dd_to_csv import main
 
             main([dd_folder, csv_folder])
@@ -203,14 +201,14 @@ def run_benchmark(
 
     runtime = time.time() - start
 
-    if verbose and res is not None:
+    if verbose:
         line = "-" * 80
         print(f"\n{line}\n{benchmark['name']}\n{line}\n\n{res.stdout}")
         print(res.stderr if res.stderr is not None else "")
     else:
         print(".", end="", flush=True)
 
-    if res is not None and res.returncode != 0:
+    if res.returncode != 0:
         print(res.stdout)
         print(f"ERROR: tool failed on {benchmark['name']}")
         sys.exit(7)

xl2times/__main__.py

@@ -387,6 +387,11 @@ def dump_tables(tables: List, filename: str) -> List:
 
 
 def run(args) -> str | None:
+    """
+    Runs the xl2times conversion.
+    :param args: pre-parsed command line arguments
+    :return comparison with ground-truth string if `ground_truth_dir` is provided, else None.
+    """
     config = datatypes.Config(
         "times_mapping.txt",
         "times-info.json",
@@ -441,6 +446,10 @@ def run(args) -> str | None:
 
 
 def parse_args(arg_list: None | list[str]) -> argparse.Namespace:
+    """Parse command line arguments
+    :param arg_list: list of command line arguments.  Uses sys.argv (default argpasrse behaviour) if `None`.
+    :return parsed arguments
+    """
     args_parser = argparse.ArgumentParser()
     args_parser.add_argument(
         "input",
@@ -478,11 +487,14 @@ def parse_args(arg_list: None | list[str]) -> argparse.Namespace:
     return args
 
 
-def main(arg_list: None | list[str] = None):
+def main(arg_list: None | list[str] = None) -> None:
+    """Main entry point for the xl2times package
+    :return: None.
+    """
     args = parse_args(arg_list)
     run(args)
 
 
 if __name__ == "__main__":
-    main()
+    main(sys.argv[1:])
     sys.exit(0)