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

BUG/TST: support user and password kwargs in remote_file_list #652

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

jklenzing
Copy link
Member

@jklenzing jklenzing commented Jan 25, 2021

Description

Addresses pysat/pysatMadrigal#5

Currently, user and password are not supported by the remote_file_list and remote_date_range methods. This updates the syntax to pass all kwargs through both routines.

Additionally, updates the test class to pass these through for packages like pysatMadrigal.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested with the tst/remote_file_list branch of pysatMadrigal. See pysat/pysatMadrigal#38. Currently works with most of the methods there, but requires some bug fixes to some of the jro_isr data products.

import datetime as dt
import pysat
import pysatMadrigal as py_mad

dmsp_f13 = pysat.Instrument(inst_module=py_mad.instruments.dmsp_ivm, tag='utd', inst_id='f13')
dl_dict = {'user': 'your name here', 'password': 'email address'}
dmsp_f13.remote_file_list(**dl_dict)

Or, to test all routines in the pysatMadrigal home directory

pytest -vs pysatMadrigal/tests/test_instruments.py::TestInstruments::test_remote_file_list

Test Configuration:

  • Python 3.8.2

Checklist:

  • Make sure you are merging into the develop (not main) branch
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • [n/a] Add a note to CHANGELOG.md, summarizing the changes

Comment on lines -3026 to -3030
# Set the user-supplied kwargs
if 'list_remote_files' in self.kwargs.keys():
kwargs = self.kwargs['list_remote_files']
else:
kwargs = {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this since it reset the kwargs with the defaults rather than user-supplied options.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with talking this out is that it means that the user/password that you can set on instrument instantiation are no longer applied here. This needs to be changed to allow that, not removed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm making a change and testing it.

@jklenzing jklenzing marked this pull request as ready for review January 26, 2021 15:52
Re-added the assignment of user kwargs at the Instrument level to the downstream methods.
Comment on lines +3039 to +3041
# Don't overwrite kwargs supplied directly to this routine
if user_key not in kwargs.keys():
kwargs[user_key] = self.kwargs[rtn_key][user_key]
Copy link
Member

Choose a reason for hiding this comment

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

@jklenzing I decided to prioritize kwargs supplied directly to the routine over those supplied to the Instrument.

Copy link
Member

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

I fixed the thing that was broke, and it works as expected locally.

@jklenzing
Copy link
Member Author

Tested this locally with the core package, as well as pysatMadrigal and pysatNASA. Everything is working as expected, with the exception of the three cases documented in pysatMadrigal. Looping in @rstoneback for a review.

@jklenzing
Copy link
Member Author

Oops. Didn't see the approval. Thanks!

@jklenzing jklenzing merged commit 47e209e into develop-3 Jan 26, 2021
@jklenzing jklenzing deleted the tst/remote_file_user branch January 26, 2021 20:17
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