-
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
Add an --ids-only
flag to easily extract ID's from a search.
#645
Comments
Note that the place this is most needed is in |
@cholmes @kevinlacaille I've got a different take on this. Instead of modifying the output of Postel's law (https://en.wikipedia.org/wiki/Robustness_principle) says that programs should be conservative in what they send, but be liberal in what they accept. I think that applies to our SDK and CLI. If I think enhancing |
@sgillies this paradigm makes a lot of sense to me. It appears the primary use case here is creating an order from search results, so enhancing |
@jreiberkyle I've got another one for you 😄 https://wiki.c2.com/?ConservationOfComplexity. Getting a comma-separated list of ids from |
To be clear this would require a pretty major change in functionality to
Well realistically right now the maximum wait would only be 500 items, since that's the max a single order could handle. #746 will hopefully address that, and will also lead us down the path of creating more than one order on a single command. Overall I do like the idea, I just don't think the argument about mixing item-types is that compelling, at least until we built the mechanism for our orders commands to handle multiple item types. Which is a direction I'm interested in, but is a non-trivial amount of work, and first I'd do the 'large orders' as that was a super clear user request, while mixing item-types from search results piped into orders is a bit theoretical, and is beyond the capabilities of our core API's. I'm right now leaning towards 'both' - get the |
It is a great point that Additionally, we can get a list of the ids with $> planet data search PSScene --limit 3 | jq -r .id
20230503_070426_25_2498
20230503_091422_18_2426
20230503_075349_44_24bb While I fully agree with discussion happening before work and not after, this ticket didn't get the benefit of clarifying the use case and thinking through the implications before the work, and the implications here are not insignificant, so I am open to discussion now (and @kevinlacaille indicated the same when we got to speak). To start off, we can limit item_types = set([i['properties']['item_type'] for i in items])
item_type = item_types.pop()
if item_types: # more than one item_type was provided
raise Exception(f'Only one item type at a time is supported, item types provided {item_types.add(item_type)}') then go on to get the ids: ids = set([i['id'] for i in items]) That all being said, it could make sense to reassess the priority of this relative to other work given the jq snippet above. @cholmes that's in your camp |
So if passing in the output of I think the 'liberal input' thing to do would be to have
?
I think some improvement is needed, since it's not just that snippet. If the dev team is all aligned on not going with |
The jq expression to make a comma separated list of strings is a little complex, but more concise than using sed and tr. The
|
@cholmes I just want to take a second to validate your main goal.
And even with Sean's snippet above, I don't think we are there yet - I for one will have to add that snippet to a cheat sheet to remember it whenever I want to use it. Here are the two approaches we are discussing, laid out in more detail Option 1 - planet orders request accepts more:
If that looks good then let's look at Currently, this is the signature of
This would need to change in the following ways:
Additional complexity:
Option 2: planet data search can output ids:
For this, we add a new boolean flag, Additional complexity:
|
I'm not strongly opposed to an Generalizing |
Thank you all for your comments, I think we have a really productive dialogue going on here! Let me gather my thoughts for a moment. At the end of the day, I want an easy path from searching for data to requesting it. The two ways of getting there mentioned are:
I really dislike the idea of having users look up how to use extra tools like I do like the simplicity of our current solution in #940 where one can simply write the following to go from search to request: Option 1: ❯ planet data search --limit 5 PSScene --ids-only \
| planet orders request - --name test --item-type PSScene --bundle analytic_udm2 However, I feel like this is likely going to be a fairly common workflow and the Option 2: ❯ planet data search --limit 5 PSScene \
| planet orders request - --name test --item-type PSScene --bundle analytic_udm2 I'm hesitant to plug this (option 1) in as a bug fix for What are your thoughts on closing this ticket and pushing for more broad inputs for |
Let's take the meta conversation to a call! I'm inclined to approve #940 but am asking for 2 changes. |
+1 on the call. However, I believe we should scrap this. Due to the accumulation, it will at the least lag, and at the worst break, if someone uses |
I was also leaning towards scrapping. Happy for call, but we may not need it. I think |
instead of a meeting, let's talk about what I propose and demonstrate in #944. Note how it takes the search result as input. The script is hacked up so there is still much work to be done but it would take only a moderate amount of effort. additionally, see what I propose and demonstrate in #945, which uses a slight variation of that script to demonstrate chunking a 500-item search result. |
Closing in favor of more comprehensive solutions like #944 and similar. |
Looking into this, is all this work to get a comma-separated list of ids from the search? Whew, we (CLI, SDK) should make it easier 😆 Maybe add an
--ids-only
flag?Originally posted by @jreiberkyle in #642 (comment)
The text was updated successfully, but these errors were encountered: