Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Issue/539 typed paginated list #656

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Issue/539 typed paginated list #656

wants to merge 5 commits into from

Conversation

bennettscience
Copy link
Contributor

Original PR (#539) from garth74:

This pull request makes two changes:

Adds type hinting to PaginatedList so that elements used in for-loops, list comprehensions, etc. are typed (more info).
Adds negative indexing to PaginatedList.

Related
Supports efforts of #435 and fixes #305 without preventing negative indexing all together (i.e., #306).

Updated to include upstream changes in develop since the original was opened in 2022.

garth74 and others added 5 commits July 25, 2022 10:05
@garth74
Added type hinting and negative indexing to PaginatedList
24f672b
@garth74
Updates test_paginated_list.py
27c2fae
@garth74
formatting
500f395
@bennettscience
Merge remote-tracking branch 'garth74/type-hinting-paginated-lists' i…
e04f995 
…nto issue/539-typed-paginated-list
@bennettscience
Bring up to date with upstream/develop
f8c7963 
This brings the PR up to speed with the current `develop` branch on
upstream. All tests passing, accounting for updates to `PaginatedList`
and `Requester` since the original PR was introduced.
@bennettscience
Copy link
Contributor Author

I think there are two things worth discussing:

  1. The typing improvements will help. I think PaginatedList is one of the harder bits of the library to get right if you've never used it before. It might help users in more complex cases avoid gotchas.
  2. My concern is about negative indexing large lists. It still has to get all of the results before you can access the results. Should we (I?) put in some more effort to address the other related issues (see PaginatedList redux #114, #114 Implement __len__() for paginated_list #237, and Add coverage for retrieving an activity stream #175.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thetwam Thetwam Awaiting requested review from Thetwam Thetwam is a code owner

@jonespm jonespm Awaiting requested review from jonespm jonespm is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Negative index on PaginatedList causes unexpected results
2 participants
@bennettscience @garth74