Fix #201 (Controllable auto-close/postpone behavior) #204
Conversation
src/teams.rs
Outdated
| // FIXME: The two following first fields don't seem to be used anywhere... | ||
| // If this is intended, document why. | ||
| name: String, | ||
| ping: String, |
There was a problem hiding this comment.
@anp Double check this FIXME :)
There was a problem hiding this comment.
I think this code might predate pub(crate) and so I didn't notice when those fields stopped being used :).
Feel free to remove them, I don't think this struct is used in API responses.
There was a problem hiding this comment.
Commented them out for now so that they are not wasting (a small bit of) RAM :)
Keeping them in rfcbot.toml in case we find future uses for them (maybe in better public messages, etc...)
rfcbot.toml
Outdated
| postpone = true | ||
|
|
||
| [fcp_behaviors."rust-lang/rust"] | ||
| #close = false |
There was a problem hiding this comment.
Is this intentionally commented out?
There was a problem hiding this comment.
This was part of the WIP to test that it doesn't matter functionally; but I'll comment it in for clarity :)
| match disposition { | ||
| FcpDisposition::Merge => { | ||
| // TODO: This one will require a lot of work to | ||
| // auto-merge RFCs and create the tracking issue. | ||
| }, | ||
| FcpDisposition::Close => { | ||
| FcpDisposition::Close if can_ffcp_close(issue) => { |
There was a problem hiding this comment.
that's a cool pattern that's new to me -- someone should write up all the fun things you can do in rust bindings
There was a problem hiding this comment.
The newest great thing is slice patterns, https://blog.rust-lang.org/2018/05/10/Rust-1.26.html
| match disposition { | ||
| FcpDisposition::Merge => {} | ||
| FcpDisposition::Close if can_ffcp_close(issue) => { | ||
| msg.push_str("\n\nBy the power vested in me by Rust, I hereby close this RFC."); |
There was a problem hiding this comment.
it would definitely be too much to add gifs to these, right?
There was a problem hiding this comment.
Since it is closing an RFC a gif could be considered gloating; but on merge, I think we can make a big splash :)
There was a problem hiding this comment.
Oh yeah, good catch -- definitely would only want to do on merge!
|
|
||
| impl RfcbotConfig { | ||
| /// Retrive an iterator over all the team labels. | ||
| pub fn team_labels(&self) -> impl Iterator<Item = &TeamLabel> { |
src/teams.rs
Outdated
| } | ||
|
|
||
| /// Are we allowed to auto-close issues after F-FCP in this repo? | ||
| pub fn ffcp_auto_close(&self, repo: &str) -> bool { |
There was a problem hiding this comment.
It's not clear to me from the function name that it's answering a question. Maybe something like should_finished_fcp_auto_close? I'm sure that without the abbreviation lots of things will be too long, but in this case the extra characters would make it much more readable.
There was a problem hiding this comment.
I went with the compromise should_ffcp_auto_close; 5 words is a bit too long for me ^,-
src/teams.rs
Outdated
| teams | ||
| }; | ||
| /// Are we allowed to auto-postpone issues after F-FCP in this repo? | ||
| pub fn ffcp_auto_postpone(&self, repo: &str) -> bool { |
There was a problem hiding this comment.
Same comment as I wrote above for ffcp_auto_close.
src/teams.rs
Outdated
| // FIXME: The two following first fields don't seem to be used anywhere... | ||
| // If this is intended, document why. | ||
| name: String, | ||
| ping: String, |
There was a problem hiding this comment.
I think this code might predate pub(crate) and so I didn't notice when those fields stopped being used :).
Feel free to remove them, I don't think this struct is used in API responses.
| { | ||
| Ok(_) => (), | ||
| Err(why) => { | ||
| error!("unable to find {} in database: {:?}", member_login, why); |
There was a problem hiding this comment.
This makes me think switching to failure would be nice, would just be a single line.
There was a problem hiding this comment.
Yeah I think we need to do something about these at some point; but I'd like to follow up with cleanup at a later point since I'm not very familiar with failure right now ;)
Fix rust-lang#201 (Controllable auto-close/postpone behavior) Former-commit-id: c466689
See title.
This might not work right now due to
-> impl Trait, in which case I'll need to bump the nightly version... ;)