Additive feature flags

[joshka]

I noticed when taking a look at the source that this doesn't build when both termion and crossterm features are enabled. It would probably be nice for development to make this possible. In general, it's worth making features additive where you can.

cargo check --all-features

    Checking keymap v1.0.0-rc.2 (/Users/joshka/local/keymap-rs)
error[E0252]: the name `parse` is defined multiple times
  --> src/backend/mod.rs:12:9
   |
6  | pub use self::crossterm::parse;
   |         ---------------------- previous import of the value `parse` here
...
12 | pub use self::termion::parse;
   |         ^^^^^^^^^^^^^^^^^^^^ `parse` reimported here
   |
   = note: `parse` must be defined only once in the value namespace of this module
help: you can use `as` to change the binding name of the import
   |
12 | pub use self::termion::parse as other_parse;
   |                              ++++++++++++++

warning: unused import: `self::termion::parse`
  --> src/backend/mod.rs:12:9
   |
12 | pub use self::termion::parse;
   |         ^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

error[E0119]: conflicting implementations of trait `Deserialize<'_>` for type `KeyMap`
   --> src/backend/termion.rs:123:1
    |
123 | impl<'s> Deserialize<'s> for KeyMap {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `KeyMap`
    |
   ::: src/backend/crossterm.rs:152:1
    |
152 | impl<'s> Deserialize<'s> for KeyMap {
    | ----------------------------------- first implementation here

error[E0592]: duplicate definitions with name `get`
  --> src/backend/crossterm.rs:46:5
   |
46 |     pub fn get(&self, key: &KeyEvent) -> Option<&T> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duplicate definitions for `get`
   |
  ::: src/backend/termion.rs:31:5
   |
31 |     pub fn get(&self, key: &KeyEvent) -> Option<&T> {
   |     ----------------------------------------------- other definition for `get`

error[E0592]: duplicate definitions with name `get_item`
  --> src/backend/crossterm.rs:59:5
   |
59 |     pub fn get_item(&self, key: &KeyEvent) -> Option<(&T, &Item)> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duplicate definitions for `get_item`
   |
  ::: src/backend/termion.rs:35:5
   |
35 |     pub fn get_item(&self, key: &KeyEvent) -> Option<(&T, &Item)> {
   |     ------------------------------------------------------------- other definition for `get_item`

Some errors have detailed explanations: E0119, E0252, E0592.
For more information about an error, try `rustc --explain E0119`.
warning: `keymap` (lib) generated 1 warning
error: could not compile `keymap` (lib) due to 4 previous errors; 1 warning emitted

[rezigned]

Hi @joshka, thanks for the feedback. This issue is one of the reason makes me reluctant to release v1. I currently have a few solutions in mind. I'd appreciate your feedback on this.

1. Export only backend (not parse)
Then end-user could choose their on <backend>::parse.

The possible nuisance is that, user may see both keymap::crossterm and crossterm (the original one) in auto-complete 😓 - or maybe this is fine?

// backend/mod.rs
#[cfg(feature = "crossterm")]
mod crossterm;

#[cfg(feature = "crossterm")]
pub use self::crossterm;

Usage

keymap::crossterm::parse("ctrl-c")

2. Make sure we can't enable more than one backend at a time

It's possible a little bit more harder to maintain if we add more backends

#[cfg(feature = "crossterm")]
mod crossterm;

#[cfg(all(feature = "crossterm", not(feature = "termion"), not(feature = "wasm")))]
pub use self::crossterm::parse;

#[cfg(feature = "termion")]
mod termion;

#[cfg(all(feature = "termion", not(feature = "crossterm"), not(feature = "wasm")))]
pub use self::termion::parse;

3. Just rename <backend>::parse function

This is the simplest, but the naming is just a bit verbose (or it's actually fine but i'm too nitpicking 😆)

#[cfg(feature = "crossterm")]
pub use self::crossterm::parse as parse_crossterm;

#[cfg(feature = "termion")]
pub use self::termion::parse as parse_termion;

[joshka]

Just a heads up, idiomatically parse() is often automatically generated from a FromStr impl instead of a manually named function.

[rezigned]

Thanks! Interesting, do you mean something like this?

// keymap_parser/src/parser.rs
impl FromStr for KeyMap {}

// Then
"ctrl-l".parse::<KeyMap>()

This would work for keymap's AST Node. For backend-specific one e.g. crossterm. I can't implement it unless I introduce newtype e.g.

struct Crossterm(crossterm::event::KeyEvent) {}

"ctrl-l".parse::<Crossterm>()

One thing I couldn't find a solution yet though. The parse_seq one returns Vec<KeyMap> (another newtype wrapper?)

[joshka]

That comment was mostly a bit of a background on wider ecosystem type things not certain if it's helpful or not.
(I certainly have to dig a bit deeper to understand this lib before commenting more - I'm at a very superficial level of understanding right now, so apologies if any of that makes you take a wrong turn...

[rezigned]

No worries, it's still behind RC-release. I'd probably change a few things before finally release it (like this one). I really appreciate you taking the time to try out the library because I can't get feedback from anywhere 😆.

[rezigned]

FYI, I've merged all refactor stuffs into the main branch. It should not raise those error anymore.