Tan-18: Remind people who haven’t sent standup msg by EOD#938
Tan-18: Remind people who haven’t sent standup msg by EOD#938angelayu0530 wants to merge 1 commit into
Conversation
Available PR Commands
See: https://github.com/tahminator/codebloom/wiki/CI-Commands |
|
|
| }; | ||
|
|
||
| const INTENTS: GatewayIntents = GatewayIntents::GUILD_MEMBERS; | ||
| const INTENTS: GatewayIntents = GatewayIntents::GUILD_MEMBERS.union(GatewayIntents::GUILD_MESSAGES); |
There was a problem hiding this comment.
nit: its more idiomatic in rust to use the | operator for bitflags. if you're curious (ik i was the first time i looked at it), the syntax is provided by the bitflags crate which allows callers to select multiple options of something.
| .await | ||
| .inspect_err(|e| println!("Error creating thread: {e:#?}")) | ||
| .map(|_| ()) | ||
| .inspect_err(|e| println!("Error creating thread: {e:#?}"))?; |
There was a problem hiding this comment.
nit: can probably avoid the error inspection now that we return if Err (which is done by the ? type automatically. see here).
| if let Err(e) = redis::client::set_standup_thread_id(thread.id.get()).await { | ||
| println!("Failed to persist standup thread id: {e:#?}"); | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
yeah honestly i handled errors really poorly, could do with a rewrite using thiserror :/ but for now, you can have it print (should probably use eprintln for errors, though ik this section was already doing it incorrectly) and then return the error via ? operator.
There was a problem hiding this comment.
on second glance, this is really not great on my part 😅
i made a lot of mistakes here that i now realize are not idiomatic. i wont block the pr on any of it, but ill try & fix it up afterwards (+ log some tickets for the team to do so as well)
| pub async fn get_standup_role_members(guild_id: u64) -> Result<Vec<Member>, Error> { | ||
| let guild = GuildId::new(guild_id); | ||
| let role = RoleId::new(ROLE_ID); | ||
| let mut result = Vec::new(); | ||
| let mut after: Option<UserId> = None; | ||
|
|
||
| loop { | ||
| let batch = guild.members(get_http().as_ref(), Some(1000), after).await?; | ||
| let batch_len = batch.len(); | ||
| after = batch.last().map(|m| m.user.id); | ||
|
|
||
| result.extend(batch.into_iter().filter(|m| m.roles.contains(&role))); | ||
|
|
||
| if batch_len < 1000 { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| Ok(result) | ||
| } |
There was a problem hiding this comment.
suggestion: this will probably get us rate-limited pretty fast. it might be a better idea to enable serenity's cache feature (we use something similar with the Java Discord lib within Codebloom)
| let guild = GuildId::new(guild_id); | ||
| let role = RoleId::new(ROLE_ID); | ||
| let mut result = Vec::new(); | ||
| let mut after: Option<UserId> = None; |
There was a problem hiding this comment.
nit: you probably didnt know this but the rust compiler can actually look ahead and infer that after must be Option<UserId> which is pretty sick! You can use this for vectors too, something along the lines of
let items = vec![];
items.push(String::from("hello world"));
// it is clear to the rust compiler that items is Vec<String> and will backfill said type.2 participants
Description of changes
Checklist before review
Screenshots
Dev
Staging