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

Add methods for setting parameter options #75

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

miabobia
Copy link
Contributor

@miabobia miabobia commented Aug 17, 2024

Implemented requested changes from Issue #74 Added unit tests for said methods as well.

Copy link
Collaborator

@chuckwondo chuckwondo left a 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
Copy link
Collaborator

@chuckwondo chuckwondo left a 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))

@chuckwondo
Copy link
Collaborator

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.

image

Copy link
Collaborator

@chuckwondo chuckwondo left a comment

Choose a reason for hiding this comment

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

Excellent!

@chuckwondo
Copy link
Collaborator

@frankinspace, I've approved this PR, but please have a look to make sure you're also happy with it.

@miabobia
Copy link
Contributor Author

miabobia commented Aug 17, 2024

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.

This is great to know for the future thank you :)

Copy link
Collaborator

@frankinspace frankinspace left a 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

@frankinspace frankinspace merged commit fb58457 into nasa:develop Aug 19, 2024
7 checks passed
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.

3 participants