-
-
Notifications
You must be signed in to change notification settings - Fork 163
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
eval: partially split up github interactions #535
base: released
Are you sure you want to change the base?
Conversation
5f72056
to
685fd94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Are those TODO
s for this PR, or a future one?
c5386e4
to
6d6fb0f
Compare
@cole-h This is pretty much everything that doesn't spill over into other parts of the code. Doesn't really matter to me to keep working in small chunks on a branch or merge them in one by one. |
I personally think it might be better to merge them all at once, if you're fine with that. The split doesn't (or shouldn't) impact anything functionality-wise, so I don't think there's a need to merge the little changes "ASAP". |
2ccdab7
to
765ca8e
Compare
765ca8e
to
f5c65fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. A suggestion and a couple questions.
ofborg/src/tasks/evaluate.rs
Outdated
@@ -495,8 +499,8 @@ fn schedule_builds( | |||
response | |||
} | |||
|
|||
pub fn make_gist<'a>( | |||
gists: &hubcaps::gists::Gists<'a>, | |||
pub fn make_gist( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not make this a method on the Client
trait that takes an &self
?
for maintainer in maint.maintainers() { | ||
if let Ok(meta) = &pull_meta { | ||
fn request_reviews( | ||
repo_client: &dyn ghrepo::Client, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, any reason not to make this a trait method for Client
?
1694f41
to
5ffedd6
Compare
5ffedd6
to
1886b2d
Compare
1886b2d
to
71ed184
Compare
No description provided.