Skip to content

Spliting update_next_page method#30

Merged
alexbourret merged 12 commits intobug/sc-104770-error-message-w-non-jsonfrom
bug/sc-104935-rfc5988-takes-priority-on-offset-pagination
Dec 8, 2022
Merged

Spliting update_next_page method#30
alexbourret merged 12 commits intobug/sc-104770-error-message-w-non-jsonfrom
bug/sc-104935-rfc5988-takes-priority-on-offset-pagination

Conversation

@alexbourret
Copy link
Copy Markdown
Collaborator

No description provided.

@shortcut-integration
Copy link
Copy Markdown

This pull request has been linked to Shortcut Story #104935: RFC5988 takes priority on offset pagination.

@alexbourret alexbourret added the bug Something isn't working label Oct 19, 2022
Copy link
Copy Markdown

@liamlynch-data liamlynch-data left a comment

Choose a reason for hiding this comment

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

I think splitting the method out for the 3 types of pagination was an awesome idea - it makes it much clearer what is going on.

So it's definitely getting there, but I was still quite confused about a lot of things and I think it needs a bit of fine tuning. I have made various suggestions/observations. Obviously we can talk about it - I may well have misunderstood things.

Also as these changes impact all of the pagination methods I will need to do some more testing around it when the changes are made - so meanwhile I will think about that and have a look at the existing unit and integration scenarios.

Comment thread python-lib/pagination.py
Comment thread python-lib/pagination.py Outdated
Comment thread python-lib/pagination.py Outdated
Comment thread python-lib/pagination.py Outdated
Comment thread python-lib/pagination.py
Comment thread python-lib/pagination.py Outdated
Comment thread python-lib/pagination.py Outdated
Comment thread python-lib/pagination.py Outdated
Comment thread python-lib/pagination.py
Comment thread python-lib/pagination.py Outdated
Copy link
Copy Markdown

@liamlynch-data liamlynch-data left a comment

Choose a reason for hiding this comment

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

The code looks good now and I have done some pretty exhaustive testing around it (details in shortcut card).

So I'm approving, but it would be nice if @StanislasBertrand is able to take a look also.

Copy link
Copy Markdown

@StanislasBertrand StanislasBertrand left a comment

Choose a reason for hiding this comment

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

Overall code LGTM, It reads quite easily, methods have a single responsibility, clear name, the logic of configure_paging defining the update_next_page makes sense.

Some ideas to make it potentially a bit clearer if you have the time some day :

  • There are 2 ways to initialise pagination, with a config and wit arguments to configure_paging. But in the plugin the config is never used so why keep it ? it makes the init more confusing.

  • pagination has a lot of attributes which can make it hard to follow what does what. A lot of the attributes of pagination are actually not really pagination attributes the state of the current "page". For instance data_is_list, could maybe just be passed as argument to has_next_page ? Or maybe it could be interesting to have just one attribute of pagination that is current_page_state dict and that stores all of these kind of atttributes

…output

Bug/sc 110446 recipe fails w int output
@alexbourret alexbourret merged commit ee2a320 into bug/sc-104770-error-message-w-non-json Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants