Skip to content

Remove filter requirement for data quick search, planet data filter cli command outputs empty filter by default #842

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

Merged
merged 6 commits into from
Feb 23, 2023

Conversation

jreiberkyle
Copy link
Contributor

@jreiberkyle jreiberkyle commented Jan 25, 2023

Related Issue(s):

Closes #600 #704 #637

Proposed Changes:

For inclusion in changelog (if applicable):

  1. Data client CLI and API no longer require filter to be specified, and will use an empty filter (no filtering) if it is not. Filter moved from a positional argument for the CLI and API to an optional key word argument, --filter for the CLI and search_filter for the API.
  2. Data filter CLI command now outputs an empty filter by default, it no longer includes permissions and standard quality filters by default, these are easily added with the --permissions and --std-quality flags

Not intended for changelog:

  1. Update CLI docs to use new interface. Change wording some to discuss permissions and standard quality filters not being defaults but being easy to add. @JuLeeAtPlanet FYI

Diff of User Interface

Planet data filter

Old behavior:

planet data filter outputs a permissions and standard quality filter by default

> planet data filter
{"type": "AndFilter", "config": [{"type": "PermissionFilter", "config": ["assets:download"]}, {"type": "StringInFilter", "field_name": "quality_category", "config": ["standard"]}]}

to get an empty filter use

> planet data filter 
{"type": "AndFilter", "config": []}

New behavior:
planet data filter outputs an empty filter by default

> planet data filter
{"type": "AndFilter", "config": []}

to get a permissions/std-quality filter:

> planet data filter --permission --std-quality
{"type": "AndFilter", "config": [{"type": "PermissionFilter", "config": ["assets:download"]}, {"type": "StringInFilter", "field_name": "quality_category", "config": ["standard"]}]}

Planet quick search, CLI

Old behavior:

planet data search PSScene filter.json applies filter to search, returns at most 100 results

planet data search PSScene errors out due to FILTER being a required argument

New behavior:

planet data search PSScene -filter filter.json applies filter to search

planet data search PSScene returns 100 (default value of limit) results

Planet quick search, API

Old behavior:

data_client.search(['PSScene'], filter)

applies filter to search

data_client.search(['PSScene'])

Errors out due to search_filter being a required argument

New behavior:

data_client.search(['PSScene'], search_filter=filter)

applies filter to search

data_client.search(['PSScene'])

Returns 100 (default value of limit) results

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:
@cholmes @JuLeeAtPlanet

@jreiberkyle jreiberkyle changed the title Remove filter requirement for data quick search Remove filter requirement for data quick search, planet data filter cli command outputs empty filter by default Jan 25, 2023
@jreiberkyle
Copy link
Contributor Author

@julee could you look at content changes?
@cholmes could you look at usage pattern?
@kevinlacaille could you do a code review?
TY!

@jreiberkyle jreiberkyle marked this pull request as ready for review January 25, 2023 21:10
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.

Looks great, works great in my limited testing.

I think the only thing in here that will be subtly confusing, especially in evolution from the current thing, is that:

planet data filter --number-in cloud_percent 4 | planet data search PSScene won't actually get the filter applied. You need to remember to add --filter -. If you've got any ideas on how to nudge people towards that it could be good. Like can we detect if they're attempting to pipe something and warn them that it won't be applied?

@jreiberkyle
Copy link
Contributor Author

@cholmes hmm... yeah, that's a hard one to protect against. There is the option of prompting the user for confirmation (overridden with an -y option) when a search is run without a filter. Requires an opt-in for searching without a filter.

@cholmes
Copy link
Member

cholmes commented Jan 30, 2023

Could it just raise a little warning when searching without a filter?
Like just says 'executing search with no filter'. I don't think we need to keep that warning around forever, just for a few releases to help people adopt new behavior.

Even better would be if we could detect if there's anything being piped in. Like if it detects some attempted pipe then it would raise that message (or an even stronger warning, since it wouldn't come up every time)

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.

Everything looks good to me! I can confirm that all tests pass on 3.10 and 3.11.

@jreiberkyle
Copy link
Contributor Author

@cholmes I don't know of a way to check for an attempt to pipe in. Or, if there is a way, it would take a lot of work. I'm also not sure of how to output a warning that wouldn't mess up any further processing of the search results - like if we pipe it to jq then jq will probably croak on the warning message before it can process the results that are the actual outputs.

@cholmes
Copy link
Member

cholmes commented Feb 2, 2023

I'm also not sure of how to output a warning that wouldn't mess up any further processing of the search results - like if we pipe it to jq then jq will probably croak on the warning message before it can process the results that are the actual outputs.

Ah, I had been thinking that a warning maybe wouldn't go to stdout in the same way, but you're probably right. Ok, sounds like no easy solution - seems like we should just ship it and be sure to document it all well. Maybe a note / warning somewhere in the docs, and in the release notes, to notify people to watch out for piping without the '-'. New users shouldn't have any trouble with it, and we only have a small number of users today, so it should be fine.

@jreiberkyle jreiberkyle merged commit fb20a0f into main Feb 23, 2023
@jreiberkyle jreiberkyle deleted the data-quick-search-no-filter-600 branch February 23, 2023 04:09
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.

Don't require a filter for a quick search.
3 participants