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

Add CLI command to validate scenario data file from definitions #419

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dc-almeida
Copy link
Collaborator

@dc-almeida dc-almeida commented Oct 22, 2024

First step towards #404

@dc-almeida dc-almeida added the enhancement New feature or request label Oct 22, 2024
@dc-almeida dc-almeida self-assigned this Oct 22, 2024
@dc-almeida dc-almeida marked this pull request as ready for review October 22, 2024 15:42
Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition @dc-almeida (sorry for jumping in here with a review).
Couple of comments in line below.

nomenclature/cli.py Outdated Show resolved Hide resolved
nomenclature/cli.py Outdated Show resolved Hide resolved
nomenclature/cli.py Outdated Show resolved Hide resolved
@dc-almeida
Copy link
Collaborator Author

Thanks for the hawk-eye as always!

@phackstock
Copy link
Contributor

Thanks for the hawk-eye as always!

You're welcome, always easier to spot these things in other people's work :D

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick changes @dc-almeida.
Good to be merged from my side, in principle.
However, as mentioned in #419, I think the name of the command might be misleading and I think it should include an option to specify the dimensions that should be validated.

nomenclature/cli.py Outdated Show resolved Hide resolved
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@phackstock
Copy link
Contributor

Before we go ahead with the merge if I understood our discussion in #419 correctly, this PR is still missing a dimensions option so that you can specify which dimension you want to validate, correct @danielhuppmann?

@danielhuppmann
Copy link
Member

I have a suggestion: we merge as is, and then @dc-almeida does a follow-up PR adding the dimensions for this CLI, and also extending the tests for the validate-scenario and validate-project to make sure that all three cases work as expected:

  • explicit dimensions argument (this is currently tested in test_cli_custom_dimensions_runs())
  • if not given, use dimensions argument set from nomenclature.yaml (not sure if this is tested)
  • if also not given, check all available folders

@phackstock
Copy link
Contributor

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants