-
Notifications
You must be signed in to change notification settings - Fork 98
Add AssetStatusBar reporter, use in data asset-wait cli command #865
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
Conversation
|
||
@property | ||
def desc(self): | ||
return f'{self.item_type} {self.item_id} {self.asset_type}' |
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.
EDIT: This is a bug which is resolved when pulling in the most recent changes from your base branch. The bug stems from item_type
being read in as a CommaSeparatedString
, but it will now be read in as a string.
Is the bar supposed to read time - item_type item_ID asset_type - status: <status>
? I currently seems to have some square brackets where item_type
should be.
When I run it on my computer I get this:
❯ planet data asset-wait PSScene 20221003_002705_38_2461 basic_udm2
00:00 - [] 20221003_002705_38_2461 basic_udm2 - status: active
assert not result.exception | ||
assert "state: active" in result.output | ||
assert "status: active" in result.output | ||
|
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.
I would check to see if all things you want to report end up in your reporting bar. Maybe something like this? This currently fails on my laptop, since item_type
isn't showing up.
assert all([item in result.output for item in [item_type, item_id, asset_type_id]]) |
I would recommend updating the reporting bar in the the i.e., Changing |
planet/cli/data.py
Outdated
max_attempts, | ||
callback=bar.update_state) | ||
click.echo(state) | ||
with AssetStatusBar(item_type, item_id, asset_type_id, |
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.
I think the issue with item_type
being reported as an empty list comes from item_type
being brought in as a CommaSeparatedString
.
When I change item_type
to a string I get:
00:00 - PSScene 20221003_002705_38_2461 basic_udm2 - status: active
If you merge in recent changes from your base branch, this should fix this!
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.
Overall, looks great! I would just take a look at my suggested fixes and update the docs. The issue with item_type
not reporting properly will be fixed once you pull in your base branch.
item_type, | ||
item_id, | ||
asset_type, |
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.
Might be worth doing this:
item_type, | |
item_id, | |
asset_type, | |
item_type: str, | |
item_id: str, | |
asset_type: str, |
This PR is to be merged after #864 and assumes the item-type argument type is changed to str.
Related Issue(s):
Part of #506
Proposed Changes:
For inclusion in changelog (if applicable):
Not intended for changelog:
Diff of User Interface
Old behavior:
New behavior:
There is a [] entry above that will actually display the item-type (aka PSScene) when the
item-type
type is changed to str.PR Checklist:
(Optional) @mentions for Notifications: