Skip to content

Conversation

@jreiberkyle
Copy link
Contributor

Related Issue(s):

Closes #813

Proposed Changes:

For inclusion in changelog (if applicable):

  1. Reverts the name change for generator functions (e.g. search()) by removing the _aiter - aka, delete that from the changelog

Not intended for changelog:

  1. Also updates usage in tests and docs

Diff of User Interface

Old behavior:

item_aiter = cl.search_aiter(['PSScene'], search_filter, limit=2)
items_list = [i async for i in item_aiter]

New behavior:

items = [i async for i in cl.search(['PSScene'], search_filter, limit=2)]

PR Checklist:

  • This PR is as small and focused as possible
  • If this PR includes proposed changes for inclusion in the changelog, the title of this PR summarizes those changes and is ready for inclusion in the Changelog.
  • I have updated docstrings for function changes and docs in the 'docs' folder for user interface / behavior changes
  • This PR does not break any examples or I have updated them

(Optional) @mentions for Notifications:

@jreiberkyle
Copy link
Contributor Author

@JuLeeAtPlanet FYI docs change
@sgillies / @kevinlacaille FYI generator function name change has been reverted

@jreiberkyle jreiberkyle merged commit 4b57b60 into main Dec 3, 2022
@jreiberkyle jreiberkyle deleted the revert-name-change-813 branch December 3, 2022 00:42
Copy link
Contributor

@kevinlacaille kevinlacaille left a comment

Choose a reason for hiding this comment

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

Awesome work, Jen! I'm a little concerned about large one-liners, especially towards maintenance and readability. However, overall, this is a great PR and I really appreciate your efforts on reverting a lot of work you had just done!

download_filter = filters.permission_filter()
search_results = await client.search(["PSScene"], filters.and_filter([date_filter, cloud_filter, download_filter]))
return [item async for item in search_results]
return [item async for item in client.search(["PSScene"], filters.and_filter([date_filter, cloud_filter, download_filter]))]
Copy link
Contributor

Choose a reason for hiding this comment

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

What was purpose of shoving these two lines into a one-liner? I tend to find these code compressions confusing to read in retrospect and more difficult to maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kevinlacaille you've got a valid point. The state of the art for usage is

async for item in client.search(...):
    # do stuff

And this can also be turned into a list comprehension like [item async for item in client.search(...)].

The code on line 52 could be made easier to read if the filter construction were lifted out, maybe? Like

search_filters = filters.and_filter([date_filter, cloud_filter, download_filter])
async for item in client.search(["PSScene"], search_filters):
    # do stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

This certainly isn't worth re-opening this ticket for, but I do agree what @sgillies wrote would be easier to read and maintain in the future 👍

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.

revert name change for generator functions

4 participants