ioos / compliance-checker

Python tool to check your datasets against compliance standards
http://ioos.github.io/compliance-checker/
Apache License 2.0
108 stars 58 forks source link

Adding inputs (i.e. options incl. value) #1091

Closed sol1105 closed 2 months ago

sol1105 commented 3 months ago

Hello,

with this PR I would like to add the command line option -I / --input to specify options incl. a value, eg. --input 'checkxy:key:value' This would for example allow to specify the path to external controlled vocabularies like MIP/CMOR tables for CORDEX, CMIP, etc. If there is a simpler way to achieve this, I would be happy to learn about it :)

The argument parser for input works similar to the one already present for option. I added the inputs as parameter to CheckSuite, ComplianceChecker and BaseCheck, similar to options, with the default None. So I hope it does not interfere/conflict with any compliance-checker functionalities or existing plugins and simply serve as a means to supply options with values for specific checks.

Kind regards, Martin

sol1105 commented 2 months ago

Could you please let me know if any further actions are needed from my end? Thank you and kind regards, Martin

benjwadams commented 2 months ago

I'll take a look over the PR, thanks.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.79%. Comparing base (ee8b880) to head (f28f321). Report is 1 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1091 +/- ## ======================================== Coverage 81.79% 81.79% ======================================== Files 25 25 Lines 5212 5212 Branches 1255 1255 ======================================== Hits 4263 4263 Misses 642 642 Partials 307 307 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

benjwadams commented 2 months ago

Hi, interesting idea. What's the advantage of creating a separate CLI switch vs overloading the already existing options/-O CLI argument?

sol1105 commented 2 months ago

Hi @benjwadams , thank you for your feedback. I wanted to avoid any potential conflict with the option command line argument. I added now the possibility to provide options as -O <checker_type>:<checker_opt>[:<checker_val>]. The result will be: {checker_type: {checker_opt : checker_val or None}} instead of {checker_type: {checker_opt}} This might be considered a breaking change, but I guess when you manually define options as set() as you were used to, it will still work. And when you test for an option if checker_opt in options with options being now a dictionary rather than a set, the outcome is the same. So I do not think it will cause problems, unless in any plugin the type is checked or the options are extended.

benjwadams commented 2 months ago

Could you add a unit test or two to ensure the parsing works as intended? Looks good otherwise.

sol1105 commented 2 months ago

Thank you. I added a test, but I am not sure if I found the best way to access the function within cchecker.py.