Implement the CommandRepl trait
#746
Conversation
|
@WaffleLapkin, I think it's ready for review. |
| #[doc = include_str!("caution.md")] | ||
| /// | ||
| #[cfg(feature = "ctrlc_handler")] | ||
| pub trait CommandRepl { |
There was a problem hiding this comment.
I don't really see the reason to make a separate trait. Can't we just add methods to BotCommands?
There was a problem hiding this comment.
We could, but a separate trait is more for a separation of concerns. What it means practically is that the utils::command module doesn't depend on dispatching, which is good for compilation time. Also, CommandRepl can be founded in the same module as message REPLs, since it's a similar thing.
There was a problem hiding this comment.
which is good for compilation time
I don't think that inter crate dependency matters for compilation time that much.
There was a problem hiding this comment.
That single change probably wouldn't matter, but as we overload our modules with redundant dependencies, in conglomeration, it probably would. E.g., when you change X upon which Y depends, then the compiler has to recompile both modules.
|
I do still insist on a separate trait since it provides better system decomposition. It also follows the principle of least astonishment, since users could find it in the same place as other REPLs and not scattered throughout the whole library. |
|
Maybe a |
Implement the `CommandRepl` trait Former-commit-id: cb2298d
Resolves #740.
I think that after removing the deprecated
commands_repl_(with_listener)functions, we can merge all REPLs into a single*.rsfile, but for now, let's leave it as-is.