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

Simplify CLI option handling #54

Merged
merged 2 commits into from
Jun 28, 2021
Merged

Conversation

matthiasschaub
Copy link
Collaborator

@matthiasschaub matthiasschaub commented Jun 10, 2021

Description

Simplify CLI option handling by only allowing one option at a time to be added.
Also rename variable names to be more concise.

Checklist

  • I have updated my branch to main (e.g. through git rebase main)
  • My code follows the style guide and was checked with pre-commit before committing
  • [ ] I have commented my code
  • [ ] I have added sufficient unit and integration tests
  • I have updated the CHANGELOG.md

@matthiasschaub matthiasschaub force-pushed the simplify-cli-option-handling branch 2 times, most recently from 4defe4f to 0b3b77a Compare June 10, 2021 12:20
@Hagellach37
Copy link
Contributor

Hey looks also good to me. But I didn't check locally.

As a general question: How much of this is tested in the CI in Jenkins?

@matthiasschaub
Copy link
Collaborator Author

Hey looks also good to me. But I didn't check locally.

As a general question: How much of this is tested in the CI in Jenkins?

There are a couple of tests for the CLI:

All tests are run by the CI.
@Hagellach37 does this answer your question?

@Hagellach37
Copy link
Contributor

Hey @matthiasschaub ,
yep I think that this answers my question. I also checked locally now.

Could you also take a look at this issue #58

It is also related to the CLI parameters and thus it would fit good to address it here as well. It seems to be that there is a bug when the feature_id parameter is not set.

@Hagellach37
Copy link
Contributor

Hagellach37 commented Jun 21, 2021

Hey @matthiasschaub ,
in general the changes look good to me. Could you please check why the test_cli.py test fails. Does it also fail for you?

FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_indicator - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_indicator_custom_fid_field_int - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_indicator_custom_fid_field_str - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_report - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_report_custom_fid_field_int - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_report_custom_fid_field_str - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_get_available_regions - AssertionError: 1 != 0

@matthiasschaub
Copy link
Collaborator Author

Hey @matthiasschaub ,
in general the changes look good to me. Could you please check why the test_cli.py test fails. Does it also fail for you?

FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_indicator - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_indicator_custom_fid_field_int - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_indicator_custom_fid_field_str - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_report - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_report_custom_fid_field_int - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_create_report_custom_fid_field_str - AssertionError: 1 != 0
FAILED tests/integrationtests/test_cli.py::TestCliIntegration::test_get_available_regions - AssertionError: 1 != 0

@Hagellach37
I can not reproduce this issues (Also the CI runs successful). Is this the only test file which produces failing tests? What happens if you execute oqt list-regions?

@matthiasschaub matthiasschaub force-pushed the simplify-cli-option-handling branch 2 times, most recently from e019ef3 to 5c6dc20 Compare June 28, 2021 07:52
Hagellach37
Hagellach37 previously approved these changes Jun 28, 2021
@matthiasschaub matthiasschaub merged commit 7cb6910 into main Jun 28, 2021
@matthiasschaub matthiasschaub deleted the simplify-cli-option-handling branch June 28, 2021 12:36
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.

None yet

2 participants