Skip to content

Move v2 orders client over to async #240

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 38 commits into from
Mar 23, 2021
Merged

Move v2 orders client over to async #240

merged 38 commits into from
Mar 23, 2021

Conversation

jreiberkyle
Copy link
Contributor

@jreiberkyle jreiberkyle commented Feb 5, 2021

Switch v2 orders client to async. Also adds sophisticated order details handling (definition of products, tools, notifications, and delivery into a single order detail).

Note, this is just the python library. CLI will be implemented in #244.

Closes #238

TODOs that will be addressed in the future: make all docs markdown docs #242, and convert testing/docbuilds to nox #243

@jreiberkyle jreiberkyle self-assigned this Feb 5, 2021
@jreiberkyle
Copy link
Contributor Author

Functionality is implemented, fully tested. Todo: update documentation

@jreiberkyle jreiberkyle changed the base branch from master to v2 February 5, 2021 02:05
@jreiberkyle jreiberkyle changed the title WIP: Move v2 orders client over to async Move v2 orders client over to async Mar 3, 2021
@ericrdunham
Copy link
Contributor

For the sake of not making more noise than I already have, are there thoughts in general around typing functions and such? I'm going to stop commenting on every function for now. :)

Copy link
Contributor

@ericrdunham ericrdunham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all console functionality from models.

@jreiberkyle
Copy link
Contributor Author

@ericrdunham 'Please remove all console functionality from models.' can you be more explicit?

@ericrdunham
Copy link
Contributor

ericrdunham commented Mar 16, 2021

@ericrdunham 'Please remove all console functionality from models.' can you be more explicit?

Sure thing! The "console functionality" that I'm thinking of is anything that assumes a console or GUI in the models and the first example (and maybe the only one?) is in planet.api.models.StreamingBody.write with the call to tqdm and progress measuring/rendering.

@jreiberkyle
Copy link
Contributor Author

@ericrdunham gotcha. Let's hammer this out within the context of that one comment thread.

Copy link
Contributor

@sarasafavi sarasafavi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreiberkyle I added a comment elsewhere (lost somewhere in the depths of github, maybe) but my only request is we go ahead and pull out CLI docs pending #244 - otherwise LGTM

@jreiberkyle
Copy link
Contributor Author

great! thanks @sarasafavi , @ericrdunham and @strixcuriosus - your thoughtful reviews and understanding around such a large PR are much appreciated.

@jreiberkyle jreiberkyle merged commit a6c6d8c into v2 Mar 23, 2021
@jreiberkyle jreiberkyle deleted the v2-orders-217-async branch March 23, 2021 23:59
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.

add concurrency to the core of the client
4 participants