scikit-hep / hepconvert

BSD 3-Clause "New" or "Revised" License
8 stars 1 forks source link

`hepconvert add` CLI API broken #78

Closed matthewfeickert closed 3 months ago

matthewfeickert commented 3 months ago

In hepconvert v1.3.2 the hepconvert add CLI API is broken as it requires multiple input files but then fails when they are provided.

Minimal reproducible example:

# generate_hist_root.py
import ROOT

gauss_1 = ROOT.TH1I("name", "title", 5, -4, 4)
gauss_1.FillRandom("gaus")
gauss_1.Sumw2()
gauss_1.SetDirectory(0)
outHistFile = ROOT.TFile.Open("hist1.root", "RECREATE")
outHistFile.cd()
gauss_1.Write()
outHistFile.Close()
$ pixi global install hepconvert
✔ Installed package hepconvert 1.3.2 pyhd8ed1ab_0 from conda-forge
  These executables are now globally available:
   -  hepconvert
$ python generate_hist_root.py
$ cp hist1.root hist2.root
$ hepconvert add --help
Usage: hepconvert add [OPTIONS] DESTINATION FILES

  Hadd files.

Options:
  -f, --force BOOLEAN          Overwrite destination file if it already exists
  --append BOOLEAN             Append histograms to an existing file
  --compression TEXT           Sets compression level for root file to write
                               to. Can be one of "ZLIB", "LZMA", "LZ4", or
                               "ZSTD". By default the compression algorithm is
                               "LZ4".
  --compression-level INTEGER  Use a compression level particular to the
                               chosen compressor. By default the compression
                               level is 1.
  --skip-bad-files BOOLEAN     Skip corrupt or non-existent files without
                               exiting
  --union BOOLEAN              Adds the histograms that have the same name and
                               appends all others to the new file
  --same-names BOOLEAN         Only adds histograms together if they have the
                               same name
  -h, --help                   Show this message and exit.
$ hepconvert add output.root hist1.root hist2.root 
Usage: hepconvert add [OPTIONS] DESTINATION FILES
Try 'hepconvert add -h' for help.

Error: Got unexpected extra argument (hist2.root)

Aside: At the moment it looks like none of the CLI API is tested in CI. This can be done using pytest-console-scripts and click.testing.CliRunner. c.f. pyhf's tests for examples of using this.

zbilodea commented 3 months ago

Fixed the multiple file input issue, sorry about that! Thanks for sharing the info about the CI tests, working on adding them now!

matthewfeickert commented 3 months ago

@zbilodea Great and thanks! Can you make a patch release with this in it?