-
Notifications
You must be signed in to change notification settings - Fork 96
Get supported bundles for a given item_type #680
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
Get supported bundles for a given item_type #680
Conversation
c6028fb
to
800cbc7
Compare
Co-authored-by: Sean Gillies <[email protected]>
Co-authored-by: Sean Gillies <[email protected]>
When I run
This is due to using the I imagine the What if, instead, we to keep the CLI option logic pretty simple here. That is, the |
That's a great catch, thanks, @jreiberkyle! I'd love to find a way around the I'm not against presenting all bundle options to the users, then raising a |
Another thing to bring into consideration is that with #616, item-type and bundle will become required arguments instead of options. Not sure how, if at all, this would affect this interaction between item-type and bundle. |
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.
This seems to work as I'd expect:
> planet orders request bop boop
Usage: planet orders request [OPTIONS] ITEM_TYPE BUNDLE
Try 'planet orders request --help' for help.
Error: Invalid value for 'ITEM_TYPE': 'bop' is not one of 'MYD09GQ', 'PSScene3Band', 'SkySatCollect', 'Landsat8L1G', 'PSScene4Band', 'Sentinel1', 'SkySatScene', 'PSScene', 'REOrthoTile', 'MYD09GA', 'MOD09GA', 'Sentinel2L1C', 'REScene', 'PSOrthoTile', 'MOD09GQ'.
> planet orders request psscene boop
Usage: planet orders request [OPTIONS] ITEM_TYPE BUNDLE
Try 'planet orders request --help' for help.
Error: Invalid value for 'BUNDLE': 'boop' is not one of 'analytic', 'analytic_udm2', 'analytic_3b_udm2', 'analytic_5b', 'analytic_5b_udm2', 'analytic_8b_udm2', 'visual', 'uncalibrated_dn', 'uncalibrated_dn_udm2', 'basic_analytic', 'basic_analytic_udm2', 'basic_analytic_8b_udm2', 'basic_uncalibrated_dn', 'basic_uncalibrated_dn_udm2', 'analytic_sr', 'analytic_sr_udm2', 'analytic_8b_sr_udm2', 'basic_uncalibrated_dn_nitf', 'basic_uncalibrated_dn_nitf_udm2', 'basic_analytic_nitf', 'basic_analytic_nitf_udm2', 'basic_panchromatic', 'basic_panchromatic_dn', 'panchromatic', 'panchromatic_dn', 'panchromatic_dn_udm2', 'pansharpened', 'pansharpened_udm2', 'basic_l1a_dn'.
The one thing I'd add is some helpful text in the docstring that indicates that the valid bundle is item-type specific, and I'd also consider adding that to the error message that comes up for 'BUNDLE', something like
"Error: Invalid value for 'BUNDLE': 'boop' is not one of the following valid bundles for PsScene 'analytic', ..."
The error message bubbles up from an exception from I think for now the exception message will have to remain the same. |
Tried this out, and overall it looks pretty good. I agree that it'd be ideal if we can just return the actual valid bundles after a user puts the item type in. But the experience is 'good enough' to merge this in - if I select a wrong one it tells me on the next step. As a user I'll wonder why they didn't just tell me, but I'll get to the right spot. So let's merge this in. I would like to see if there's any way to do a better warning, but can just make a ticket for the future - it's no longer 'must fix this', more in 'that'd be nice'. |
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.
Looks great, thanks for all your hard work on this Kevin.
Thanks for all of your reviews, @sgillies, @jreiberkyle, and @cholmes! I agree the UX isn't the best it could be, but we'll need to think hard about a workaround with or without |
PR #677 did not fully address this issue, however this PR should cover all the bases!
Validating
item_type
beforeproduct_bundle
, so that the item type compatibility exception will propagate before the bundle validation exception.sepcs.validate_item_type()
now just checks if inputitem_type
is a valid input (i.e., if part of all item type)specs. validate_bundle()
now checks if the input product bundle is a valid product bundle (i.e., if part of all product bundles), then checks if item type is compatible with the given product bundlespecs.validate_supported_bundles()
specs.validate_supported_bundles()
specs.validate_bundle()
andspecs.validate_item_type()
item-type
andbundle
are now both arguments, instead of optionsis_eagar
flag toitem_type
, which 1. requiresitem_type
to be an option and 2. requires an item type to be given when enteringplanet orders request --help
.Previous behaviour (1): Incompatible item type with given bundle would return all compatible item types with given bundle.
New behaviour (1): Incompatible item type with given bundle now returns all compatible bundles with given item type.
Previous behaviour (2): Only
--name
provided prompted user to enter a bundle.New behaviour (2): Only
--name
provided now prompts the user to enter an item type and now provides all choices for item types.Closing #618, #698