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

Implement message reply #84

Merged
merged 12 commits into from
Jan 13, 2021
Merged

Implement message reply #84

merged 12 commits into from
Jan 13, 2021

Conversation

ilya-zlobintsev
Copy link
Contributor

@ilya-zlobintsev ilya-zlobintsev commented Jan 12, 2021

This pull request adds the ability to reply to a given Privmsg using the reply-parent-msg-id IRC tag.

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Copy link
Collaborator

@RAnders00 RAnders00 left a comment

Choose a reason for hiding this comment

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

I would like the function to be more reusable, aka a method signature similar to pub async fn say_in_reply(&self, channel_login: &str, in_reply_to_message_id: Option<String>, message: String) -> ...

Then also, you should make it so passing None makes it behave exactly like say, and then rewrite say() to use say_in_reply() with a parameter of None. The current function just sends the message out like the privmsg message which might execute commands on accident. Replying to a message with a command wouldn't make sense, so that's why I want it to behave like say() and also be called say_in_response().

The original method signature that accepts a PrivmsgMessage could then be added as pub async fn reply_to_privmsg(&self, reply_to: &PrivmsgMessage, message: String) -> ... which delegates to say_in_reply.

src/client/mod.rs Outdated Show resolved Hide resolved
src/client/mod.rs Outdated Show resolved Hide resolved
@ilya-zlobintsev
Copy link
Contributor Author

Running rustfmt turns the method signatures into multi-line ones that don't fit with the rest of the functions at all, so I suppose the check can be ignored.

@RAnders00
Copy link
Collaborator

If the method signatures are long then it‘s okay to wrap them like rustfmt wants to. I want to keep the formatting consistent with rustfmt, that‘s why there is the CI check.

src/client/mod.rs Outdated Show resolved Hide resolved
src/client/mod.rs Outdated Show resolved Hide resolved
src/client/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@RAnders00 RAnders00 left a comment

Choose a reason for hiding this comment

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

I took the liberty to directly edit the changelog to my preferences. There really should be a documented format for the changelog, I apologize. The version heading gets added when a release is made, and I typically try to put relevant issues/PRs as links to the end of the changelog entry.

All looks good now! If you're happy with what I changed the changelog entry too then I'm looking forward to merging this! ❤️

@ilya-zlobintsev
Copy link
Contributor Author

👍

@RAnders00
Copy link
Collaborator

Also, something that I just thought of is that it's not possible to do a /me message in response with the current methods, but I'll probably add that separately when I add the /me sending function.

@RAnders00 RAnders00 merged commit 7381e75 into robotty:master Jan 13, 2021
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.

None yet

2 participants