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 CoC update message... command #89

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add CoC update message... command #89

wants to merge 3 commits into from

Conversation

technetos
Copy link
Member

This PR adds the CoC update message... command to update the content of the existing welcome message

@technetos technetos requested a review from khionu June 11, 2021 02:24
src/welcome.rs Outdated Show resolved Hide resolved
@technetos technetos requested a review from khionu June 19, 2021 21:56
.ok_or("unable to retrieve message param")?;

let welcome_message_is_not_cached = {
let data = args.cx.data.read();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say this function was ran twice in parallel. This check could be hit twice before the message is stored in data, which could cause parallel attempts to populate it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely correct, nice catch, this is a problem and I will address it.

.message(args.cx, u64::from_str(&message_id)?)?;

cache_welcome_message(args.cx, message)?;
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of failing, maybe we should somehow have it post the message? Might be tricky, ergonomically speaking, because of the lack of awareness of which channel to post in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants