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

Redesign client command support. #16

Closed
aatxe opened this issue Feb 24, 2015 · 7 comments
Closed

Redesign client command support. #16

aatxe opened this issue Feb 24, 2015 · 7 comments

Comments

@aatxe
Copy link
Owner

aatxe commented Feb 24, 2015

We should redesign Command to address a few problems. Firstly, current conversion into a Command results in loss of data (specifically, you lose the message prefix). Secondly, the code to convert messages into commands is a huge mess. Thirdly, the code to convert commands into messages is also a huge mess. This stuff could do with a lot of love. It's hard to maintain as it stands.

For some initial design thoughts, macros might be useful in cleaning up the conversion from messages. The rest of the issues may be improved by the following design (perhaps with better names?):

struct Command {
    message: Message,
    detail: CommandDetail
}

enum CommandDetail<'a> {
    // RFC commands...
    PRIVMSG(&'a str, &'a str)
    // more commands...
}

/cc @filipegoncalves @retep998

@aatxe
Copy link
Owner Author

aatxe commented Feb 24, 2015

So, as @retep998 pointed out to me on IRC, there's no way to actually construct such a Command. So, we'll have to look at different design options. This may just involve finding a way to make from_message more maintainable.

@isomorpheme
Copy link

My 2 cents as someone stumbling upon this: shouldn't Command really be a part of Message, instead of Command being a separate type that can be converted into a Message? Something like:

struct Message {
    prefix: Option<String>,
    command: Command
}

enum Command {
    NICK(String),
    QUIT(Option<String>))
    // etc...
}

As far as I've understood from the RFC, every IRC message is a command as well - chatting in a channel is really just privmsg-ing that channel. So from the data point of view, having only a Message type containing Commands makes more sense. From the usability point of view though, being able to just send Commands to a server makes more sense. But for that case, perhaps you could impl Into<Message> for Command in such a way that the command just gets copied to the message struct, and the prefix left blank. Seems much simpler IMO.

@aatxe
Copy link
Owner Author

aatxe commented Jun 14, 2015

The motivation for keeping them separate before was that there are potentially custom or otherwise unknown commands. This is perhaps less of an issue now that we have mostly complete IRCv3 support, but one example would be Twitch chat has a custom command. One way around this would be to add an Unknown command to the enumeration and to just have it default to either a Vec or perhaps a Vec and a String (separating out the suffix).

@ghost
Copy link

ghost commented Jul 16, 2015

One of the IRCv3-WG's unofficial policies regarding unknown commands is that they should just be passed to the server straight through—most server implementations these days return numerics for unknown commands, so the client is unlikely to be surprised by a lack of response. Additionally, it makes the need for aliases to work with e.g. services more efficiently entirely obsolete (aside from those servers which don't provide their own aliases, but that's comparatively rare).

A generic 'raw' command, through which unknown-to-the-crate commands flow by default, would probably be most appropriate.

@aatxe
Copy link
Owner Author

aatxe commented Sep 10, 2015

I'll try to work on this at some point, but I'd accept any help. I'd like to implement @ijks's proposal with the Raw command as the default as per @Aerdan's suggestion.

@ketsuban
Copy link

Looks like that commit didn't actually close this issue. Github's magic is a little too magical at times, it seems.

@aatxe
Copy link
Owner Author

aatxe commented Jan 30, 2016

I believe it's because it's on a separate branch that I haven't merged in yet. I'm waiting a bit to merge because I'm going to do some other major breaking changes, and I don't want to merge it into master until all the changes are ready for release.

@aatxe aatxe closed this as completed in 2eb0e63 Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants