-
Notifications
You must be signed in to change notification settings - Fork 22
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 methods for setting parameter options #75
Conversation
… the functions in test_queries.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mamamia96, I apologize, but upon further reading in the CMR Search API docs, I realize that the solution to this is more involved than I initially thought. Further, attempting to cover all of the details within this PR review will not only be cumbersome, but won't provide sufficient visibility to others, so I'm going to provide extensive details in a comment on #74 , and request changes to this PR.
Once you read the additional details in the original issue, you can come back and make the appropriate changes to this PR. Since the effort may be more than you're comfortable with (since you mentioned that you're pretty new to contributing), I'm happy to continue providing guidance, or to directly take on the changes myself, if that's what you wish.
…. Enabled doctests. Query's options member is now a defaultdict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mamamia96, looks great!
One more thing. Please add the following immediately under the Unreleased
section at the top of CHANGELOG.md
:
### Added
- Add `Query` method `option` for setting parameter options as described both in
[CMR Search API Parameter Options](https://cmr.earthdata.nasa.gov/search/site/docs/search/api.html#parameter-options)
and in other sections of the CMR Search API documentation, thus supporting
other parameter options that are not covered in that particular section of the
documentation. ([#74](https://github.com/nasa/python_cmr/issues/74))
Nicely done! For future reference, when changes are requested for your PR, you make the requested changes, and you're ready for the reviewer to review all of your updates, click the circular pair of arrows icon to prompt the reviewer to take another look (i.e., the icon that the big red arrow below points to). Since you might very well make multiple new commits to address the requested changes, it is not obvious to the reviewer when you're done with your changes until you press that icon, which explicitly indicates that you're done and ready for a re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
@frankinspace, I've approved this PR, but please have a look to make sure you're also happy with it. |
This is great to know for the future thank you :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is great; love the doc tests too
Implemented requested changes from Issue #74 Added unit tests for said methods as well.