Its quite tedious to use either parts of it in another thread. Perhaps drop the Writer wrapping the W with an Arc/Mutex and instead use a channel internally

I want to flatten the layout out.

for example:

    match msg {
        twitchchat::twitch::Message::PrivMsg(
            msg @ twitchchat::twitch::commands::PrivMsg { channel, .. },
        ) if channel == "museun" => {
            do_something(&msg);
        }
        _ => {}
    }

could just be:

    match msg {
        twitchchat::Message::Privmsg(msg @ twitchchat::PrivMsg { channel, .. })
            if channel == "museun" =>
        {
            do_something(&msg);
        }
        _ => {}
    }

with local imports:

    use twitchchat::twitch::commands::PrivMsg;
    use twitchchat::twitch::Message;
    match msg {
        Message::PrivMsg(msg @ PrivMsg { channel, .. }) 
            if channel == "museun" => 
        {
            do_something(&msg);
        }
        _ => {}
    }

compared to:

    use twitchchat::*;
    match msg {
        Message::Privmsg(msg @ PrivMsg { channel, .. }) 
            if channel == "museun" => 
        {
            do_something(&msg);
        }
        _ => {}
    }

This would probably break the current api, I can duplicate it all (just pub use whatever::* in the root namespace), but I'd rather just move everything. Having the root crate act like a prelude is definitely an ergonomic gain.

I still want the irc module to be in its own namespace so it doesn't clash and isn't emphasized for usage.

for example:

(also, don't need to export the UserConfigBuilder)

Although you are able to access a badge of a user, you may not compare its kind to another, because only Badge but not BadgeKind is reexported here:

IRC Messages are limited to 512 bytes (including the trailing \r\n). When a user does a .send() or a .raw() there's no checks to make sure this fits within that 512 bytes, which also includes all the Command and Target information.

One solution would be to just provide a split function that the core .write() uses to break up long lines into multiple lines. With this split function, a rate-limit method could be added which can adhere to the Twitch guidelines for message sending limitation.s

from the docs

To implement rate limiting, a token bucket could be used -- or even a timer wheel.
Other parts of the crate need to bring in chrono for doing "time-like" operations. Keeping track of the last send per window would count against the available tokens.

A major design decision and question would be what to do when we've exceeded this. Do we block the thread?

Such as:

let next_window = some_calculation();
std::thread::sleep(std::time::Duration::from_millis(next_window));

Or do we have the caller decide when to 'try' again? By returning some "rate limit" style error and internally buffer it, we can allow the caller to figure out what to do w.r.t to blocking and retrying.

    enum RateLimit {
        Limited {
            reqs_remain: usize,
            preferred_time: std::time::Duration,
        },
        Continue, // or None or something
    }

    // drain queue here first (FIFO (recursive call this function maybe))

    let next_window = some_calculation();
    if self.is_too_soon(next_window) {
        self.queue.push(their_data);
        return Ok(RateLimit::Limited {
            reqs_remain: self.queue.len(),
            preferred_time: self.next_best_time(),
        });
    }
    Ok(RateLimit::Continue)

This implies it should be the full prefix, :nick!user@host

But here it just returns the nick portion of the prefix

Here:

It should also join() a channel before the .run()

see:

That should just be the user name, as a String.
There are probably other messages that are like this.
Its not incorrect, but highly inconvenient.

Also provide an example on how to implement it, and how to use it (and the default SyncReaderAdapter implementation).

hashbrown is now in the stdlib
parking_lot's changes are important

just a way to get PrivMsgs into a crossbeam channel

struct Client {
    writer: twitchchat::Writer,
    recv: channel::Receiver<Message>,
    user: twitchchat::LocalUser,
    handle: std::thread::JoinHandle<()>,
}
impl Client {
    fn connect(nick: &str, channel: &str, token: &str) -> Result<Self, Error> {
        use twitchchat::commands::PrivMsg;
        use twitchchat::*;

        let config = UserConfig::with_caps()
            .token(token)
            .nick(nick)
            .build()
            .unwrap();

        let stream = TcpStream::connect(TWITCH_IRC_ADDRESS) //
            .map_err(crate::Error::Connect)?;

        let (read, write) = sync_adapters(stream.try_clone().unwrap(), stream);
        let mut client = Client::new(read, write);

        let (tx, rx) = channel::unbounded();

        struct ReadMessage {
            send: channel::Sender<crate::Message>,
        }
        impl Handler for ReadMessage {
            fn on_priv_msg(&mut self, msg: std::sync::Arc<PrivMsg>) {
                let msg = msg.into();
                // this should shut down the client if we cannot send
                // instead of unwrapping, that way the caller can just drop this channel
                // and ideally it would close the connection, thus return from the thread below
                self.send.send(msg).unwrap(); 
            }
        }
        client.handler(ReadMessage { send: tx });

        client.register(config).map_err(crate::Error::Register)?;
        let user = client.wait_for_ready().map_err(crate::Error::NotReady)?;

        let writer = client.writer();
        // TODO we need to know if its a bad channel by waiting for the joined message
        writer.join(channel).unwrap();

        let handle = std::thread::spawn(move || {
            let _ = client.run(); // this needs to signal that we've lost the connect
        });

        Ok(Self {
            writer,
            recv: rx,
            user,
            handle,
        })
    }
}

an "Easy" path should be added

let client = EasyClient::connect(nick, token)?; // name pending
let writer = client.writer();
for msg in client {
    match msg {
        _ => writer.stuff()?
    }
}

EasyClient (tm) could allow for a builder pattern to set up handlers/callbacks to mimic libraries in other languages. and then just return an iterator over the filtered messages

or just pseudo-iterator adapters..

let client = Client::new(nick, token).with(commands::PrivMsg).filter(commands::Join);
// the iterator will return only PrivMsgs and Joins

another idea:
Writer should have a shutdown method that'll (gracefully) shut down the client

a new Client that isn't generic over a ReadAdapter would also make it much simpler just to pass it around instead of dealing with (the likely realistic) Client<SyncReadAdapter<TcpStream>>

This will allow for better coherence. The blanket Into from std lib for coherence breaks the current attempt.

Currently something like the following has problems:

struct S;
impl Display for S {
    fn fmt(&self, f: &mut Formatter<'_>) -> Result {
        write!(f, "foobar")
    }
}

fn foo(d: impl Into<Channel>) { 
    let ch = d.into();
}

foo("foobar"); // works
foo(S{}); // does not work
foo(S{}.to_string()); // works

And a bit of bikeshed. ToChannel vs AsChannel (e.g. consuming or borrowing). With the way the Writer works, it doesn't need ownership because its just going to format a string in the end, so everything can be borrowed which would be concatenated with some static data.

Ideally, there should only ever be 1 "reader" in the client, but many "writers".

We could do a ReadHalf/WriteHalf style API like in Futures, where ReadHalf moves and isn't clonable, but WriteHalf is clonable and thread safe.

Right now: its a bad idea (tm) to do the read-operations concurrently on cloned Clients. The write stuff is fine, but we can reduce lock contention by using some stuff from crossbeam. Cloned writers would just publish to an internal lock-free queue and a single internal writer will pull from it.

One major problem with simply splitting it into ReadHalf/WriteHalf would be that read_message() writes back to the client, internally, to handle things like PINGs automatically. This would have to be considered if we were to "split the responsibility". The ReadHalf could have its own internal clone of a WriteHalf for example.

Perhaps a channels-based API should be discussed. Replace the inspect/filter functions with channels, and then operate internally on read/write channels.

For instance, the tags.get("message") stuff is generally wrong, it should be using the message field from the irc message

these on the Writer:

fn ban(&self, username: &str, reason: Option<&str>) -> Result<(), Error>;
fn op(&self, username: &str) -> Result<(), Error>;
fn unmod(&self, username: &str) -> Result<(), Error>;
fn unban(&self, username: &str) -> Result<(), Error>;
fn untimeout(&self, username: &str) -> Result<(), Error>;
fn vip(&self, username: &str) -> Result<(), Error>;
fn unvip(&self, username: &str) -> Result<(), Error>;
fn whisper(&self, username: &str, message: &str) -> Result<(), Error>;

and probably more

Although I'm not really following semvar (1.0 will be stable -- as the Rust ecosystem has settled on), major.minor.point is still convention.

Minor versions are API breakages, where Point versions are stable fixes.

..along side of the display_name

struct Bar {
    user: String,
    data: String,
}

fn foo(msg: commands::PrivMsg) -> Bar{
    // except for indv. fields (or some .into() style function)
    let (user, data) = msg.take_whatever();
    Bar {
        user, 
        data
    }
}

Currently, only references are obtainable from the types. even if you own it,
so there should be an IntoInner trait that is applied to all of the types in the crate that allow for extraction

Something like this, maybe. (either tuples or dedicated Owned$name structs)

trait IntoInner {
    type Inner;
    fn into_inner(self) -> Self::Inner;
}

struct Foo {
    foo: String, 
    bar: String 
}

impl IntoInner for Foo {
    type Inner = (String,String); 
    fn into_inner(self) -> Self::Inner {
        (self.foo, self.bar)
    }
}

A higher level API would allow for things like client.on(|msg: Hosted| {}) which would involve parsing the PRIVMSG for the hosting string and using a different message type.

Either to a separate set of crates, or as a workspace.

This should be replaced with just

pub fn join_many<'a, I>(&self, channels: I) -> Result<(), Error>
    where
        I: IntoIterator + 'a,
        I::Item: AsRef<str> + 'a,
{
    for channel in channels {
        self.join(channel)?;
    }
    Ok(())
}

This will allow for easier integration with AsyncRead/AsyncWrite-style clients, using something like this:

pub trait Adapter {
    // owned for simplicity but perhaps a &'a [u8] (and use of the `bytes` crate)
    // <= 1024 string ending with \r\n
    fn read_line(&mut self) -> std::io::Result<String>;  
    // <= 512 buffer ending with \r\n
    fn write_line(&mut self, data: &[u8]) -> std::io::Result<usize>; 
}

And something like will allow people to use other irc parsing crates to provide the twitchchat::irc::types::Message that'll be used with the twitchchat::Message types:

// TODO: perhaps ToOwned / Borrowed (Cow) for these
// @tags :prefix command args :data\r\n
pub trait ToMessage {
  fn tags(&self) -> Option<&str>;
  fn prefix(&self) -> Option<&str>;
  fn command(&self) -> Option<&str>;
  fn args(&self) -> Option<&[&str]>;
  fn data(&self) -> Option<&str>;
}

// twitchchat::Message
impl Message {
  fn parse(msg: impl ToMessage) -> Self {
    let tags = msg.tags();
    let prefix = msg.prefix();
    let command = msg.command();
    let args = msg.args();
    let data = msg.data();

    // do parsing here
  }
}

(which will allow for removing the whole irc module and providing an optional one that implements this trait)

this'll return #foobar rather than foobar

it should probably return a

and that type should have an .as_str() along with PartialEq for &str and String

There should be more (and actually useful) logging.

involving two .into() is annoying, and it complicates the ser/de stuff
make a concrete struct with an enum to denote the named color.

should be something like

struct Color {
    name: TwitchColor,
    rgb: RGB,
}
// with TwitchColor being
enum TwitchColor {
    Blue,
    BlueViolet,
    CadetBlue,
    Chocolate,
    Coral,
    DodgerBlue,
    Firebrick,
    GoldenRod,
    Green,
    HotPink,
    OrangeRed,
    Red,
    SeaGreen,
    SpringGreen,
    YellowGreen,
    Turbo,
}

Perhaps generate it from the lib.rs documentation:

for example, when you get hosted: :[email protected] PRIVMSG museun :shaken_bot is now hosting you.

this will cause a panic. which probably means a lot more of those unwraps are bad.

its not listed here
https://dev.twitch.tv/docs/irc/commands/
rather its only listed
https://dev.twitch.tv/docs/irc/tags/

but after some testing, it'll only send it if both are enabled.

Providing a trait for this will allow for more flexibility in what can be used inside of the client

crate::extension::WriterExt::join_many_limited is broken unless visited from the re-export at crate::WriterExt

I think it'll be this way until intra-doc links are ready for stable?

The crate is using 0.2 but 0.3 is out .
0.3.0 is the version that is the basis for the replacement of std::collections::HashMap
the repo also changed to https://github.com/rust-lang/hashbrown

recently changes to BadgeKind requires a version bump for adding new variants
its recommended for public enums to have a variant such as

    #[doc(hidden)]
    __Nonexhaustive,

so the consumer of the enum has to have a _ => () arm in their pattern matching of the enum
this allows variants to be added to the enum in the future, in a non-breaking way.

also one day (tm), rust-lang/rust#44109 will be usable

finally, perhaps hide the fields of some (all?) of the structs to aid in future proofing.
these changes should decided before the next API break (0.4.0)

I don't, personally, like the twitchchat::twitch::commands::PrivMsg level of nesting. the top-level twitchchat::twitch could be reduced. Commonly used things can either be re-exported into the root, a prelude-style module.

It should be less verbose or maybe provide some 'canned' configs where the user just has to provide their nick and their oauth token.

    let config = twitchchat::UserConfig::builder()
        .commands()
        .membership()
        .tags()
        .nick(&opts.nick)
        .token(pass)
        .build()
        .expect("valid config"); // why is this an option?

could just be

    let config = UserConfig::all_caps()
        .with_nick("foo")
        .with_token("bar")
        .build();

And instead of returning an Option (which I assume most people would just unwrap anyway), panic because this should be a hard error. The UserConfig type won't be built dynamically, so if a required field is missing (nick and/or token) it should yell at the user rather then continuing.

the .on() method should take a trait in

and the trait should be implemented for

Fn(Message)
FnMut(Message)
Fn(Message, Client)
FnMut(Message, Client)
// and others in the future

this enables way more flexibility

..still waiting on this CI check

in this section:

technically it'd be a

pub type Result = std::result::Result<(), Error>;
// with -> Result {

or a

pub type Result<E> = std::result::Result<(), E>;
// with -> Result<Error> {

both of which are non-idiomatic, though

The on method should return a unique Id that can be stored and used later to remove that filter.
While this is being done, determine whether the name should be a combination of on/off or filter/remove_filter

or have a From<HashMap<String,String>> impl at the very least

Channel should implement from 'From's

this should be valid:
let ch = client.writer().join("foo").unwrap()

join should block until we get a message that we joined

challenges:
twitch's irc is a bit weird, so a lot of the times this is only possible with specific capabilities
and somteimes twitch doesn't reply like a normal server would

and set here:

At the moment this crate (0.7.0) is broken because of this oversight.

Until 0.7.1 is out, one could register for Ping and respond with the proper Pong to avoid this