Skip to content

Extend CrateSelect to allow an arbitrary number or explicit list of crates #461

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Oct 3, 2019
Merged

Extend CrateSelect to allow an arbitrary number or explicit list of crates #461

merged 9 commits into from
Oct 3, 2019

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Sep 20, 2019 •
edited
Loading

Resolves #460.

Make CrateSelect take an arbitrary number as part of a top-{n} or random-{n} crate specification or a comma-separated list of crates. Also allows craterbot comments to specify a list of crates via a URL (e.g. crates=https://gist.githubusercontent.com/ecstatic-morse/837c558b63fc73ab469bfbf4ad419a1f/raw/example-crate-list).

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 20, 2019 •
edited
Loading

Marked as a draft because I need to document the new functionality, test this somehow and fix the TODO. I'm hoping for comments on whether this (somewhat hacky) approach is acceptable or not.

@ecstatic-morse ecstatic-morse force-pushed the crate-select branch 3 times, most recently from b648b96 to eb08842 Compare September 20, 2019 19:33
@ecstatic-morse ecstatic-morse mentioned this pull request Sep 20, 2019
Merged
@ecstatic-morse ecstatic-morse changed the title Level up CrateSelect Extend CrateSelect to allow arbitrary numbers or an explicit list of crates Sep 20, 2019
@ecstatic-morse ecstatic-morse changed the title Extend CrateSelect to allow arbitrary numbers or an explicit list of crates Extend CrateSelect to allow an arbitrary number or explicit list of crates Sep 20, 2019
@ecstatic-morse ecstatic-morse marked this pull request as ready for review September 24, 2019 02:50
@ecstatic-morse
Copy link
Contributor Author

@pietroalbini this still adds no tests, but I have tried everything but the link form of list: locally. I added docs and resolved the TODO by adapting the code for CrateSelect::Demo above. I'm removing the draft status, but let me know if I need to mock reqwest for testing or something.

@ecstatic-morse ecstatic-morse mentioned this pull request Sep 24, 2019
Merged
@pietroalbini pietroalbini self-assigned this Sep 24, 2019
bors added a commit that referenced this pull request Sep 24, 2019
@bors
Auto merge of #463 - ecstatic-morse:link-fix, r=pietroalbini
3710ce0 
Fix broken links in bot usage docs

Saw these while working on #461.
pietroalbini
pietroalbini requested changes Sep 24, 2019
View reviewed changes
Copy link
Member

@pietroalbini pietroalbini 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 mostly good, but why is the URL handled just by the bot and not by the CLI? The two should have the same functionality.

As for the test, I guess it'd be nice to get URL fetching tested. Can you see how hard would it be to add mocking support to utils::http? If it's too hard or cumbersome I'm not sure it's worth it.

bors added a commit that referenced this pull request Sep 24, 2019
@bors
Auto merge of #463 - ecstatic-morse:link-fix, r=pietroalbini
b28943f 
Fix broken links in bot usage docs

Saw these while working on #461.
@bors
Copy link
Contributor

bors commented Sep 24, 2019

☔ The latest upstream changes (presumably #453) made this pull request unmergeable. Please resolve the merge conflicts.

@ecstatic-morse
Copy link
Contributor Author

I added parsing tests for CrateSelect and DeferredCrateSelect. I think this obviates the need to mock get_sync, since the code on either side of it is now pretty thoroughly tested. I also made the CLI take DeferredCrateSelect as well.

@pietroalbini
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2019

📌 Commit 7d2588d has been approved by pietroalbini

bors added a commit that referenced this pull request Oct 1, 2019
@bors
Auto merge of #461 - ecstatic-morse:crate-select, r=pietroalbini
f1b805b 
Extend `CrateSelect` to allow an arbitrary number or explicit list of crates

Resolves #460.

Make `CrateSelect` take an arbitrary number as part of a `top-{n}` or `random-{n}` crate specification or a  comma-separated list of crates. Also allows craterbot comments to specify a list of crates via a URL (e.g. crates=https://gist.githubusercontent.com/ecstatic-morse/837c558b63fc73ab469bfbf4ad419a1f/raw/example-crate-list).
@bors
Copy link
Contributor

bors commented Oct 1, 2019

⌛ Testing commit 7d2588d with merge f1b805b...

@bors
Copy link
Contributor

bors commented Oct 1, 2019

💔 Test failed - checks-travis

@ecstatic-morse
Expand impl of FromStr for CrateSelect
c7a28b1
@ecstatic-morse
Pass CrateSelect by reference
5aedd65
@ecstatic-morse
Make CrateSelect more flexible
3e4adeb 
This lets `CrateSelect` take arbitrary numbers as arguments to `top-`
and `random-` (while preserving `small-random`) as well as a list of
crates to run directly.
@ecstatic-morse
Allow webhook to link to list of crates
412d304
@ecstatic-morse
Document crate selections available with craterbot
e36da6f
@ecstatic-morse
Copy link
Contributor Author

The new craterbot check alias needed to take a DeferredCrateSelect parameter as well. This should work now.

@eddyb eddyb mentioned this pull request Oct 2, 2019
Closed
@pietroalbini
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2019

📌 Commit 68958a8 has been approved by pietroalbini

@bors
Copy link
Contributor

bors commented Oct 3, 2019

⌛ Testing commit 68958a8 with merge 8b6119a...

bors added a commit that referenced this pull request Oct 3, 2019
@bors
Auto merge of #461 - ecstatic-morse:crate-select, r=pietroalbini
8b6119a 
Extend `CrateSelect` to allow an arbitrary number or explicit list of crates

Resolves #460.

Make `CrateSelect` take an arbitrary number as part of a `top-{n}` or `random-{n}` crate specification or a  comma-separated list of crates. Also allows craterbot comments to specify a list of crates via a URL (e.g. crates=https://gist.githubusercontent.com/ecstatic-morse/837c558b63fc73ab469bfbf4ad419a1f/raw/example-crate-list).
@bors
Copy link
Contributor

bors commented Oct 3, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: pietroalbini
Pushing 8b6119a to master...

@bors bors merged commit 68958a8 into rust-lang:master Oct 3, 2019
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Oct 3, 2019 •
edited
Loading

Thanks @pietroalbini. Could you ping me somewhere when this gets deployed?

@petrochenkov petrochenkov mentioned this pull request Jun 21, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietroalbini pietroalbini pietroalbini approved these changes

Assignees

@pietroalbini pietroalbini

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define an experiment on a subset of crates from the github bot
3 participants
@ecstatic-morse @bors @pietroalbini