Skip to content

show warning when limit exceeds search api max#3967

Merged
mislav merged 2 commits intocli:trunkfrom
despreston:des/3839-warn-limit
Sep 13, 2021
Merged

show warning when limit exceeds search api max#3967
mislav merged 2 commits intocli:trunkfrom
despreston:des/3839-warn-limit

Conversation

@despreston
Copy link
Contributor

@despreston despreston commented Jul 9, 2021 •
edited
Loading

Fixes #3839
Closes #3839

@mislav mislav self-assigned this Jul 20, 2021
@mislav mislav added the enhancement a request to improve CLI label Aug 4, 2021
mislav
mislav suggested changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thanks for your patience! This looks good, but could the approach be extended to also cover issue and repo search too? #4176

@despreston despreston force-pushed the des/3839-warn-limit branch from e48304e to 83b5f4e Compare August 23, 2021 21:33
@despreston
Copy link
Contributor Author

despreston commented Aug 23, 2021

I moved the ErrSearchAPIMaxLimit error to /api/client.go in order to access that in other commands. Pls lmk if that's the wrong place. That /api pkg is a little disorganized IMO; I think a lot of the files in there could be consolidated into one /api/api.go file.

@despreston despreston requested a review from mislav August 31, 2021 20:52
@mislav mislav force-pushed the des/3839-warn-limit branch from 83b5f4e to 53f625b Compare September 6, 2021 14:03
@mislav mislav force-pushed the des/3839-warn-limit branch from 53f625b to 7b486a4 Compare September 6, 2021 14:05
mislav
mislav approved these changes Sep 6, 2021
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

@despreston Your approach worked but I've found it to be a code smell that information about the search cap was passed via error values rather than by other means. Additionally, warnings such as these should be printed to stderr regardless of isTerminal status. Finally, I've found that listing issues would print a warning about the search cap even when the Search API was not used.

I've pushed my own approach to solve these issues, which I think is simpler as well. Please review!

@mislav mislav force-pushed the des/3839-warn-limit branch from 7b486a4 to 619333a Compare September 6, 2021 14:09
@despreston
Copy link
Contributor Author

despreston commented Sep 7, 2021

lgtm

@mislav mislav merged commit 6f3b1eb into cli:trunk Sep 13, 2021
@despreston despreston deleted the des/3839-warn-limit branch September 13, 2021 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mislav mislav mislav approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mislav mislav

Labels

enhancement a request to improve CLI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Warn about Search index cap when user has requested more than a 1000 results

2 participants

@despreston @mislav