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

Add limit parameter to search command (fix issue #2402) #2439

Merged
merged 1 commit into from Mar 6, 2016

Conversation

Projects
None yet
4 participants
@jespino
Copy link
Contributor

jespino commented Mar 4, 2016

No description provided.

@jespino

This comment has been minimized.

Copy link
Contributor Author

jespino commented Mar 4, 2016

This have a problem with a limit greater that 100 (this limit is in the crates.io API). Here I can do 3 things, allow only limits values between 1 to 100, allow any value and let it crash when value is greater than 100, or query as pages as needed for satisfy the limit.

Any suggestion?

@jespino jespino force-pushed the jespino:issue-2402 branch from b6406b9 to 6840406 Mar 4, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 4, 2016

This looks good to me, I'm fine letting the maximum limit get defined by crates.io so we don't hardcode it in two places.

@alexcrichton alexcrichton assigned alexcrichton and unassigned wycats Mar 4, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 4, 2016

Looks like a test may need to get updated as well?

@jespino

This comment has been minimized.

Copy link
Contributor Author

jespino commented Mar 5, 2016

The crates.io maximum is about API results pagination, I think isn't directly related with the maximum on cargo search command, but I think 100 results is enough for the command line interface. I will allow only values 1..100.

@jespino jespino force-pushed the jespino:issue-2402 branch from 6840406 to 4fefe73 Mar 5, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 6, 2016

Looks like the tests may still be failing?

@jespino jespino force-pushed the jespino:issue-2402 branch from 4fefe73 to 53c9374 Mar 6, 2016

@jespino

This comment has been minimized.

Copy link
Contributor Author

jespino commented Mar 6, 2016

Now tests are fixed, I think the problem is a temporary problem obtaining a package.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 6, 2016

@bors: r+ 53c9374

Thanks!

bors added a commit that referenced this pull request Mar 6, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 6, 2016

⌛️ Testing commit 53c9374 with merge 075e824...

@bors

This comment has been minimized.

@bors bors merged commit 53c9374 into rust-lang:master Mar 6, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.