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.

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

Refactor paginator #425

Merged
merged 32 commits into from
Feb 26, 2024
Merged

Refactor paginator #425

merged 32 commits into from
Feb 26, 2024

Conversation

InnocentBug
Copy link
Collaborator

@InnocentBug InnocentBug commented Feb 21, 2024

Description

This refactors the paginator.
Instead of having the user manually page through the pages of search results, this improved paginator is a python iterator.

So users, can just loop through the results with for node in paginator as you would expect from python.
A new change is also that the paginator now directly returns nodes instead of JSON that can be converted to nodes.

Changes

Tests are adjusted to the new design.

Known Issues

  • 2 search tests fail at the moment.
  • One because an invalid material is returned upon search
  • One because the bigsmile search connection is timing out.

Notes

Tagging @duboyal for visibility.

Checklist

  • My name is on the list of contributors (CONTRIBUTORS.md) in the pull request source branch.
  • I have updated the documentation to reflect my changes.
  • My code changes have been verified by automated tests and pass all relevant test scenarios.

@InnocentBug InnocentBug requested a review from brili February 21, 2024 01:21
@InnocentBug InnocentBug self-assigned this Feb 21, 2024
Copy link

trunk-io bot commented Feb 21, 2024

Merging to develop in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

@InnocentBug InnocentBug changed the base branch from main to develop February 21, 2024 01:22
@InnocentBug InnocentBug force-pushed the refactor-paginator branch 2 times, most recently from bed4509 to b0a432c Compare February 21, 2024 15:13
brili
brili previously approved these changes Feb 22, 2024
@InnocentBug InnocentBug mentioned this pull request Feb 23, 2024
3 tasks
@InnocentBug
Copy link
Collaborator Author

Merge #429 first.

@InnocentBug InnocentBug requested review from duboyal and brili February 26, 2024 19:13
@InnocentBug InnocentBug marked this pull request as ready for review February 26, 2024 19:13
Copy link

@duboyal duboyal left a comment

Choose a reason for hiding this comment

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

approval for capsule

and all other results are on the next pages.
Using the Paginator object, the user can simply and easily flip through the results of the search.
The details, that results are listed as pages are hidden from the user.
The pages are automatically requested from the API as needed.
Copy link

Choose a reason for hiding this comment

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

ok so this comment is the heart of the paginator functionality

self._api = api
self._vocabulary = {}
self._db_schema = self._get_db_schema()

Copy link

Choose a reason for hiding this comment

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

is this a constructor for a data schema class where it preloads all the schemas for the nodes and make that accessible throughout other parts of the code? did this constructor not really exist before ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is not really a constructor, since it is just getting the JSON data as a dict from the API.
This call existed before, but was in a different location.
I moved it now into the constructor of the DataSchema class, since a DataSchema without loaded JSON schema is not useful.

@InnocentBug InnocentBug merged commit 7f3f664 into develop Feb 26, 2024
13 checks passed
@InnocentBug InnocentBug deleted the refactor-paginator branch February 26, 2024 23:16
api,
url_path: str,
page_number: Union[int, None],
query: str,
):
Copy link

Choose a reason for hiding this comment

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

does this object init function take a page number ? just curious in a use case of this instantiation how does a user know what page number to pass in unless there is also a way to get a number of pages?

Copy link
Collaborator Author

@InnocentBug InnocentBug Feb 27, 2024

Choose a reason for hiding this comment

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

It does take a page number, because some queries do not allow pages to be specified.
So in that case a user can specify None for the page.

Also the constructor of the class is mostly hidden from the user as only api.search instantiates them and returns it to the user.

We could give it a default 0 so it a user doesn't know the default is good.

Copy link

Choose a reason for hiding this comment

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

@InnocentBug could you give a quick example where the call to the paginator would look like to retrieve an already existing node? I left this comment on my sdk update as well because you left an original comment to be implementing api.search or paginator .

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.

3 participants