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

Make custom key bindings an optional Cargo feature #615

Merged
merged 7 commits into from
Apr 18, 2022
Merged

Make custom key bindings an optional Cargo feature #615

merged 7 commits into from
Apr 18, 2022

Conversation

lopopolo
Copy link
Contributor

Add a cargo feature called custom-bindings which is enabled by
default. The custom-bindings feature enables APIs which allow users of
rustyline to define custom keybindings.

Custom keybindings are the only use of radix_trie in this crate. If
the custom-bindings feature is disabled, radix_tree is not included
in rustyline.

Motivation of this commit is to reduce dependencies.

The approach I've taken in this commit is to stub out internal APIs when
custom-bindings feature is not active to minimize the number of code
changes that need to occur. Similarly, when the custom-bindings
feature is not enabled, I've added allow(dead_code) to mod bindings.

The splitting out of default functionality into a new feature that is
enabled by default I believe is a breaking change, because users may
have previously been using custom keybindings with default-features =
false.

This commit adds a step to the GitHub Actions CI to run cargo check
with no default features.

@lopopolo
Copy link
Contributor Author

Cargo tree output now looks like this:

$ cargo tree -e no-dev --no-default-features
rustyline v9.1.2 (/Users/lopopolo/dev/repos/rustyline)
├── bitflags v1.3.2
├── cfg-if v1.0.0
├── fd-lock v3.0.5
│   ├── cfg-if v1.0.0
│   └── rustix v0.34.2
│       ├── bitflags v1.3.2
│       ├── errno v0.2.8
│       │   └── libc v0.2.122
│       ├── io-lifetimes v0.6.1
│       └── libc v0.2.122
├── libc v0.2.122
├── log v0.4.16
│   └── cfg-if v1.0.0
├── memchr v2.4.1
├── nix v0.23.1
│   ├── bitflags v1.3.2
│   ├── cfg-if v1.0.0
│   ├── libc v0.2.122
│   └── memoffset v0.6.5
│       [build-dependencies]
│       └── autocfg v1.1.0
├── unicode-segmentation v1.9.0
├── unicode-width v0.1.9
└── utf8parse v0.2.0

@gwenn
Copy link
Collaborator

gwenn commented Apr 10, 2022

@lopopolo
Copy link
Contributor Author

thanks for the review @gwenn. I took a stab at your feedback using a similar approach I take in some other crates I maintain.

When building the docs like this:

RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --open

These methods on Editor render like this:

Screen Shot 2022-04-10 at 12 04 46 PM

Add a cargo feature called `custom-bindings` which is enabled by
default. The `custom-bindings` feature enables APIs which allow users of
`rustyline` to define custom keybindings.

Custom keybindings are the only use of `radix_trie` in this crate. If
the `custom-bindings` feature is disabled, `radix_tree` is not included
in `rustyline`.

Motivation of this commit is to reduce dependencies.

The approach I've taken in this commit is to stub out internal APIs when
`custom-bindings` feature is not active to minimize the number of code
changes that need to occur. Similarly, when the `custom-bindings`
feature is not enabled, I've added `allow(dead_code)` to `mod bindings`.

The splitting out of default functionality into a new feature that is
enabled by default I believe is a breaking change, because users may
have previously been using custom keybindings with default-features =
false.

This commit adds a step to the GitHub Actions CI to run `cargo check`
with no default features.
src/keymap.rs Outdated
Comment on lines 11 to 12
#[cfg(feature = "custom-bindings")]
mod custom_bindings {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if you'd prefer for me to use cfg_if::cfg_if! here

@gwenn
Copy link
Collaborator

gwenn commented Apr 18, 2022

@lopopolo
Copy link
Contributor Author

Oh sorry @gwenn I missed the notification on my fork

@gwenn gwenn merged commit b49a475 into kkawakam:master Apr 18, 2022
lopopolo added a commit to artichoke/artichoke that referenced this pull request Aug 2, 2022
`rustyline` 10.0.0 includes a breaking change where `Editor::new` now
returns result. The REPL propagates this error as an unhandled readline
error, which will exit the REPL with an error message.

This is a handcrafted version of @dependabot's PR in #1999 to address
this breaking change.

`rustyline` 10.0.0 includes several commits (a few of which I authored)
that reduce the number of deps pulled in a bunch:

- kkawakam/rustyline#613
- kkawakam/rustyline#614
- kkawakam/rustyline#615
- kkawakam/rustyline#623
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.

2 participants