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

Features/add cli option #147

Merged
merged 15 commits into from
Aug 18, 2022

Conversation

luciecastella
Copy link
Contributor

@luciecastella luciecastella commented Aug 10, 2022

Closes #71

This adds a features allowing users to optionally type in the dimensions to check non-default dimensions in the validation of a project.
This is currently done with (edit) : --dimensions "['variable', 'region', 'foo']"

Lucie added 4 commits August 9, 2022 17:52
Cli accepts any other arguments but nothing is done with it yet
Way to call the option: 
--dimensions=variable,region,foo
@luciecastella
Copy link
Contributor Author

For now it works with --dimensions=variable,region,foo
But I could also try multiple entries as Philip suggested, I do not know which one is the more elegant

@danielhuppmann
Copy link
Member

Tried to run this, but it seems that tests are now failing if a dimensions argument is not given (so this argument is now treated as required, not optional). Also, the test-definitions look fine but are not actually executed yet in any test.

About the approach:
I see a risk of unintuitive behavior when users add a whitespace (which one would do when following pep8-style), so the following would fail

nomenclature validate-project . --dimensions=foo, variable

I still think that something like

nomenclature validate-project . --dimensions="['foo', 'variable']"

would be preferable.
Maybe the following could be helpful? https://stackoverflow.com/a/47730333

--dimensions "['variable', 'region', 'foo']"
@luciecastella
Copy link
Contributor Author

I spent quite some time on an error because apparently ' and " are not behaving the same: --dimensions "['variable', 'region', 'foo']" works fine, --dimensions '["variable", "region", "foo"]' does not. Am I the only one who does not know that? If not, I will remember to indicate this clearly for the users, that might confuse people too.

And yes I will work on the tests execution.

@danielhuppmann
Copy link
Member

Am I the only one who does not know that?

I didn't know that either, but if there are some examples (in the docs and the tests), it should be relatively straightforward for users to "automatically" use the right structure...

@luciecastella
Copy link
Contributor Author

Yes OK I will do that

@luciecastella
Copy link
Contributor Author

Hello!
New problem: I have issues giving the new cli arguments in the CliRunner invoke.
This is what I did
image
And result_valid ends up with an exit_code 2 even though the apparently same command line creates no error on my terminal. Any clue on how to write the invoke? I tried different things and looked online but I cannot figure out how it could be right

tests/test_cli.py Outdated Show resolved Hide resolved
@luciecastella luciecastella marked this pull request as ready for review August 17, 2022 14:57
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.

Looks pretty good overall. Some comments below in line.
One more general comment about testing the failure states of the cli is that maybe we want to test for more than just the exit code. We want to assure that the cli fails in the way intended and not just that it fails in any way. What do you think @danielhuppmann, should this be added here or should this be subject of a possible follow-up PR?

nomenclature/testing.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@danielhuppmann
Copy link
Member

We want to assure that the cli fails in the way intended and not just that it fails in any way. What do you think @danielhuppmann, should this be added here or should this be subject of a possible follow-up PR?

Agree with you @phackstock in principle, but let's do that as a follow-up PR.

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 addressing all the comments @luciecastella.
Good to be merged from my side.

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.

Thanks @luciecastella, nice work!

@danielhuppmann danielhuppmann merged commit acd3a4b into IAMconsortium:main Aug 18, 2022
@luciecastella luciecastella deleted the features/add-cli-args branch August 22, 2022 11:33
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.

Add "dimensions" arg to validation CLI
3 participants