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

Task 32 / Add pagination to list commands #72

Merged
merged 8 commits into from
Oct 27, 2021
Merged

Conversation

grananda
Copy link
Contributor

@grananda grananda commented Oct 26, 2021

Closes #32

@grananda grananda self-assigned this Oct 26, 2021
@grananda grananda linked an issue Oct 26, 2021 that may be closed by this pull request
@grananda
Copy link
Contributor Author

We need members and collaborators

Copy link
Member

@jordeu jordeu left a comment

Choose a reason for hiding this comment

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

It will be nice to show that there are more items when you are not showing all the rows. But, if I'm not wrong, this is not possible because the API paginated response do not includes the total number of available rows. If this is the case, maybe it is worth to open an issue to track this problem at Tower API.

Copy link
Member

@swampie swampie left a comment

Choose a reason for hiding this comment

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

Can we please add some tests about pagination? We could check that the urls are built correctly

# Conflicts:
#	src/main/java/io/seqera/tower/cli/commands/teams/AbstractTeamsCmd.java
@jordeu
Copy link
Member

jordeu commented Oct 27, 2021

Ok, I've seen that to add the validate = false to the ArgGroup is not the solution.

If you don't set the validate = false the help shows this:

Options:
  -f, --filter=<startsWith>   Show only pipeline's runs that it's name starts with the given word
*     --page=<page>           Page to display to display (default is 1)
*     --offset=<offset>       Rows record's offset (default is 0)
*     --max=<max>             Maximum number of records to display (default is 100)
*     --no-max                Show all records
  -h, --help                  Show this help message and exit.
  -V, --version               Print version information and exit.

Which is not correct because the ArgGroup arity by default is 0..1 that means optional. And in fact they're optional, so it's only a visualization problem of the help usage.

If we set validate = false then the help usage is correct and all the * disappear, but then picocli is not checking that you only enter one option of the mutually exclusive group.

We've hit this bug: remkop/picocli#1380

Maybe for now the solution can be to keep the validate = false and "manually" check the mutually exclusivity and throw an exception. What do you think?

Copy link
Member

@swampie swampie left a comment

Choose a reason for hiding this comment

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

Let's implement the manual check to overcome the validate=false issue and then we are good to go for me

@jordeu jordeu merged commit 2505f12 into master Oct 27, 2021
@jordeu jordeu deleted the task-32-pagination branch October 27, 2021 10:00
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.

Use pagination when available
3 participants