-
Notifications
You must be signed in to change notification settings - Fork 93
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
Added --ids-only option #940
base: main
Are you sure you want to change the base?
Conversation
I realized I never explained this in the original ticket, but the key for this is that the output can be directly piped into the orders request command. So it needs to remove the " and replace the newlines with commas. So output should be like |
if ids_only: | ||
item_ids.append(item['id']) | ||
else: | ||
echo_json(item, pretty) |
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.
@kevinlacaille In #645 I'm pushing back a little on this feature request, but if we did it, this is completely sound. Append to list or print to terminal, this is easy to read and understand. It's symmetry in action.
@@ -77,16 +77,20 @@ async def list(ctx, state, limit, pretty): | |||
@translate_exceptions | |||
@coro | |||
@click.argument('order_id', type=click.UUID) | |||
@click.option('--ids-only', is_flag=True, help='Returns only the item IDs.') |
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.
@kevinlacaille #645 is only about getting a list of ids from search. Let's not extend to the orders CLI here. Can you remove the changes to planet/cli/orders.py?
@@ -309,13 +310,18 @@ async def search(ctx, item_types, filter, limit, name, sort, pretty): | |||
parameter will be applied to the stored quick search. | |||
""" | |||
async with data_client(ctx) as cl: | |||
|
|||
item_ids = [] |
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.
In #645 we've recognized that search results may cover multiple item_types and that mixing their ids together sets a user up for failure. @kevinlacaille what would you think about this?
item_ids = [] | |
if ids_only and len(item_types) > 1: | |
raise ClickException("item id output is not allowed when multiple item types have been searched.") | |
item_ids = [] |
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.
@kevinlacaille I'm in favor, but would like to see 2 changes.
I am against merging this at this time. Lets have more discussion. |
Related Issue(s):
Closes #645
Proposed Changes:
For inclusion in changelog (if applicable):
ids-only
, which returns only the item IDs from a search or order, to the following functions:data search
,data search-run
,orders get
Not intended for changelog:
Diff of User Interface
Old behavior:
For example, data search
New behavior:
data search
data search-run
orders get
PR Checklist:
(Optional) @mentions for Notifications: