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

change search-create name/filter options/parameters to match search UI #883

Merged
merged 7 commits into from
Mar 22, 2023

Conversation

jreiberkyle
Copy link
Contributor

@jreiberkyle jreiberkyle commented Mar 21, 2023

Related Issue(s):

Closes #884

Proposed Changes:

For inclusion in changelog (if applicable):

  1. Breaking change: Bring the UI of planet data search-create and DataClient.create_search() inline with planet data search and DataClient.search() by moving name and filter to required options in the CLI and rearranging the order of the parameters to (item_types, search_filter, name) in the Python library.

Not intended for changelog:

Diff of User Interface

Old behavior:

planet data search-create name PSScene filter.json
dataclient.create_search(name, ['PSScene'], search_filter)

New behavior:

planet data search-create PSScene --filter filter.json --name search_name
dataclient.create_search(['PSScene'],  search_filter=search_filter, name=name)

PR Checklist:

  • This PR is as small and focused as possible
  • If this PR includes proposed changes for inclusion in the changelog, the title of this PR summarizes those changes and is ready for inclusion in the Changelog.
  • [] I have updated docstrings for function changes and docs in the 'docs' folder for user interface / behavior changes
  • This PR does not break any examples or I have updated them

(Optional) @mentions for Notifications:
There are no docs for these functions so there was nothing to update. #885 addresses adding them.

@jreiberkyle
Copy link
Contributor Author

@cholmes FYI

Copy link
Member

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Haven't had time to check out and try things out, but the description of changes looks right on, so as long as this is doing what's described / expected I'm +1.

@cholmes
Copy link
Member

cholmes commented Mar 22, 2023

Hrm, just realized the search-update should also be updated for consistency. Elsewhere (#887) I mentioned planet data stats could also use an update. I think that one is 'nice to have', but probably not essential. But search-update seems like it really should have the same behavior. I'm only trying it out now - I had thought it might just take JSON instead of having arguments, but just realized it has the arguments, and it'd be weird to have them different.

Copy link
Contributor

@kevinlacaille kevinlacaille left a comment

Choose a reason for hiding this comment

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

All looks great, just a few small, very minor comments.

planet/cli/data.py Outdated Show resolved Hide resolved
planet/cli/data.py Show resolved Hide resolved
tests/integration/test_data_cli.py Outdated Show resolved Hide resolved
@kevinlacaille
Copy link
Contributor

Awesome work, Jen!

@jreiberkyle jreiberkyle merged commit c70ea07 into rc3dev-fixes Mar 22, 2023
@jreiberkyle jreiberkyle deleted the saved-search-filter branch March 22, 2023 18:47
@jreiberkyle jreiberkyle mentioned this pull request Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants