ReproNim / reproman

ReproMan (AKA NICEMAN, AKA ReproNim TRD3)
https://reproman.readthedocs.io
Other
24 stars 14 forks source link

ENH: provide a more informative KeyError by listing known ones #517

Closed yarikoptic closed 4 years ago

yarikoptic commented 4 years ago

To provide an informative error message like:

$> reproman run --follow -r local --sub local --orc bogus --bp "pl=1,2" echo '{p[pl]}'     
2020-05-25 21:18:07,772 [ERROR  ] 'bogus'. Known keys: plain, datalad-pair, datalad-pair-run, datalad-local-run [collections.py:__getitem__:34] (UnknownKeyError) 

instead of

2020-05-25 21:18:51,813 [ERROR  ] 'bogus' [run.py:__call__:406] (KeyError) 

which would require user to RTFM instead of fixing up the invocation

TODOs

codecov[bot] commented 4 years ago

Codecov Report

Merging #517 into master will increase coverage by 0.10%. The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage   89.47%   89.58%   +0.10%     
==========================================
  Files         148      149       +1     
  Lines       12290    12305      +15     
==========================================
+ Hits        10997    11023      +26     
+ Misses       1293     1282      -11     
Impacted Files Coverage Δ
reproman/support/collections.py 64.28% <64.28%> (ø)
reproman/support/jobs/orchestrators.py 91.86% <100.00%> (+0.01%) :arrow_up:
...eproman/interface/tests/test_backend_parameters.py 100.00% <0.00%> (+4.34%) :arrow_up:
reproman/tests/skip.py 97.75% <0.00%> (+4.49%) :arrow_up:
reproman/distributions/tests/test_venv.py 98.07% <0.00%> (+10.57%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b6528a1...7a965b4. Read the comment docs.

yarikoptic commented 4 years ago

Overall @kyleam - do you think it is a worthwhile approach or we should just use another design pattern? e.g. is there a particular reason we have not listed known submitters etc as choices within run interface _params_ definitions?

kyleam commented 4 years ago

e.g. is there a particular reason we have not listed known submitters etc as choices within run interface _params_ definitions?

No, I think that was just an oversight. The _params_ listing is a little funky to accommodate --list without repeating information (and I think it'd be a mistake to try to pack that information into the command docstring), but I don't see any reason the keys of SUBMITTERS and ORCHESTRATOR couldn't be used as the value for choices.

kyleam commented 4 years ago

gh-520 adds constraints to --orc and --sub, if you'd prefer to go that direction.

yarikoptic commented 4 years ago

Let's consider gh-520 sufficient for now, and I will revisit this one if I run into another use case where this additional protection would be needed.