Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jul 15, 2024
Merged

Conversation

sol1105
Copy link
Contributor

@sol1105 sol1105 commented Jun 12, 2024

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

- adding command line option -I/--input
- adding inputs to CheckSuite, ComplianceChecker and BaseCheck
@sol1105
Copy link
Contributor Author

sol1105 commented Jul 9, 2024

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

@benjwadams benjwadams self-requested a review July 11, 2024 16:04
@benjwadams
Copy link
Contributor

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

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@benjwadams
Copy link
Contributor

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

@sol1105
Copy link
Contributor Author

sol1105 commented Jul 12, 2024

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
Copy link
Contributor

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

@sol1105
Copy link
Contributor Author

sol1105 commented Jul 15, 2024

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

@benjwadams benjwadams merged commit 8f96172 into ioos:develop Jul 15, 2024
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants