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

Pont 3 api key to constructor #11

Merged
merged 5 commits into from Oct 6, 2020
Merged

Conversation

pont-us
Copy link
Member

@pont-us pont-us commented Oct 5, 2020

This PR adds cds_api_url and cds_api_key parameters to the CDSDataOpener and CDSDataStore constructors. Previously, the CDS API URl and key could only be specified via environment variables or a configuration file.

Add the cds_api_url and cds_api_key arguments to CDSDataOpener and use them
(if supplied) when instantiating the CDS API client. To facilitate testing of
this feature, add a last_instantiated_client field to CDSDataOpener and url
and key fields (and corresponding constructor arguments) to CDSClientMock.
@pont-us pont-us requested review from dzelge and TonioF October 5, 2020 14:05
Copy link
Contributor

@TonioF TonioF left a comment

Choose a reason for hiding this comment

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

The tests work nicely. However, I found it a bit hard to figure out the differences between the three new tests 'test_client_url_and_key_parameters', 'test_client_url_and_key_environment_variables' and 'test_client_url_and_key_rc_file'. I recommend that you at least document the tests a bit so that we know why e.g. to test the file access you pass only the client_class to the opener.
Also, it seems that the tests do not consider any preexisting environment variables and may overwrite those. This should be prevented.
Finally, I'd recommend to create a new TestClass for the new tests (e.g., ClientUrlTest) and use setUp()- and tearDown()-functions instead of the two static methods you have used.

@pont-us
Copy link
Member Author

pont-us commented Oct 6, 2020

@TonioF Thanks for the suggestions -- all implemented. I've put the new tests into a new ClientUrlTest class, added docstrings, added setUp and tearDown functions, and done some other refactoring to remove duplication and improve code readability. Existing environment variables are now properly saved and restored.

@pont-us pont-us merged commit 56c5ee7 into master Oct 6, 2020
@pont-us pont-us deleted the pont-3-api-key-to-constructor branch October 7, 2020 14:44
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

3 participants