add cwd aware hinter #647

p00f · 2023-10-17T04:21:50Z

codecov · 2023-10-17T07:21:00Z

Codecov Report

Merging #647 (94fa7d6) into main (fb9337d) will decrease coverage by 0.60%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #647      +/-   ##
==========================================
- Coverage   49.76%   49.17%   -0.60%     
==========================================
  Files          41       42       +1     
  Lines        7827     7921      +94     
==========================================
  Hits         3895     3895              
- Misses       3932     4026      +94

Files	Coverage Δ
src/hinter/default.rs	`0.00% <ø> (ø)`
src/history/base.rs	`83.95% <0.00%> (-9.02%)`	⬇️
src/hinter/cwd_aware.rs	`0.00% <0.00%> (ø)`

p00f · 2023-10-17T10:27:35Z

I tried this out and it works - and I love the experience. This was the only thing I missed from fish.

Help with writing tests would be appreciated

fdncred · 2023-10-17T11:29:25Z

Nice! It would be good to have an example that demonstrates its use in the examples folder. Can you add one of those please? It would also be ideal to have a test or two in the cwd_aware.rs file itself so we can ensure this feature isn't accidentally broken.

p00f · 2023-10-17T12:24:05Z

Nice! It would be good to have an example that demonstrates its use in the examples folder. Can you add one of those please?

I tried running the existing one (cargo run --example hinter) and don't actually see hints - do you want me to just copy-paste it into a new example and just change the constructor like so:

// Create a reedline object with in-line hint support.
// cargo run --example cwd_aware_hinter
//
// Fish-style cwd history based hinting
// assuming history ["abc", "ade"]
// pressing "a" hints to abc.
// Up/Down or Ctrl p/n, to select next/previous match

use nu_ansi_term::{Color, Style};
use reedline::{CwdAwareHinter, DefaultPrompt, Reedline, Signal};
use std::io;

fn main() -> io::Result<()> {
    let mut line_editor = Reedline::create().with_hinter(Box::new(
        CwdAwareHinter::default().with_style(Style::new().italic().fg(Color::LightGray)),
    ));
    let prompt = DefaultPrompt::default();

    loop {
        let sig = line_editor.read_line(&prompt)?;
        match sig {
            Signal::Success(buffer) => {
                println!("We processed: {buffer}");
            }
            Signal::CtrlD | Signal::CtrlC => {
                println!("\nAborted!");
                break Ok(());
            }
        }
    }
}

fdncred · 2023-10-17T12:48:19Z

I think so, but you'll need to add some items to the history in order to get hints to work.

p00f · 2023-10-17T15:42:56Z

right - how do I change the cwd inside the repl though? One idea is to alternate between original cwd and std::env::home_dir() on every iteration.

fdncred · 2023-10-17T16:52:04Z

I'm not sure exactly. The create_filled_example_history() function can put data in the history. I guess you could call std::env::set_current_dir() to change directory but you'd only want to change directories to known places I think.

p00f · 2023-10-17T17:01:15Z

I tried https://paste.sr.ht/~p00f/5b683a638a097cf58f8ea4cdd7da17bf16a7e7c9 but it doesn't save the extra attributes:

create_filled_example_history()

perfect, i'll change that function to add a few more entries in / and some in std::env::home_dir(). Then I can alternate between / and home dir.

- guard CwdAwareHinter with feature flag - remove references to fish from DefaultHinter as fish is cwd aware - add example

p00f · 2023-10-17T17:29:49Z

How does the example look? I tested the example and it works as intended.

p00f · 2023-10-17T17:32:02Z

I think I need help writing tests as I'm not familiar with the code too much and there are no tests for the existingDefaultHinter

sholderbach · 2023-10-18T14:00:53Z

What is the behavior when you try to use it with FileBackedHistory? We can't just guard via feature flags as you can exchange the history at runtime as done in nushell.

p00f · 2023-10-18T14:07:12Z

On the nushell side I just use DefaultHinter when the file format is FileBackedHistory (I'm assuming engine_state is updated at runtime, so the behaviour is that it is just not used)

p00f · 2023-10-18T14:08:04Z

https://paste.sr.ht/~p00f/5752fc99ba31990710f08c2a85de897a5b15bf54#cwd.diff-L48

@sholderbach this is my nushell patch

p00f · 2023-10-18T14:13:55Z

Note that this causes no undesirable behaviour - if you change the history format you don't get hints from the other history file anyway

p00f · 2023-10-18T14:27:27Z

(I'm assuming engine_state is updated at runtime, so the behaviour is that it is just not used)

this is correct - the Hinter is created inside the REPL loop

sholderbach · 2023-10-19T11:02:23Z

My caution is driven by the fact that we have a substantial number of non-nushell users of reedline. And since we introduced the new history search API (#401) we had a few panics based on the capabilities or assumptions what info is accessible (e.g. #480).

I would like to avoid a situation where you would encounter runtime panics when the Hinter/History combination gets used.

Things should be either abundantly clear from documentation, failure should happen either at the earliest point possible or handled gracefully.

paste.sr.ht/~p00f/5752fc99ba31990710f08c2a85de897a5b15bf54#cwd.diff-L48

This would bring every user of file_format: "sqlite" over to the behavior that prioritizes hints for the current directory.
Not sure if this is universally liked. So there might be someone proposing a decoupling between the two down the line.

p00f · 2023-10-19T11:20:36Z

I see - there's no way to find out the type of History, the trait has no such methods. Do I create a HistoryType enum and the implement such a method? If I do end up doing this, why not just do away with dynamic dispatch and use enums instead?

imo I should just document that it should always be used with SqliteBackedHistory.

Not sure if this is universally liked. So there might be someone proposing a decoupling between the two down the line.

If they do, I'll add a config option. At least for now, I think everyone will love this - fish has this by default without a config option, nobody complains.

fdncred · 2023-10-19T12:53:47Z

I think everyone will love this ...

Personally, I pass on this behavior. I'll play with it and test it out, but I don't think I'll be using it. Let's make it configurable.

I think it's great to have it, and there's no doubt that some people will love it. However, I've worked on nushell long enough to know that you will never please everyone.

On a separate note, maybe I'll change my nick from "fdncred" to "everyone" then we can list the features that 'everyone' loves. 🤣

p00f · 2023-10-19T13:01:24Z

I'll play with it and test it out.

of course, I haven't created a nushell PR yet.

On a separate note, maybe I'll change my nick from "fdncred" to "everyone" then we can list the features that 'everyone' loves. 🤣

Please do, I hate when people ping @everyone on discord - this way it won't actually ping everyone 😛

fdncred · 2023-10-19T13:03:14Z

Please do, I hate when people ping @everyone on discord - this way it won't actually ping everyone.

LOL - me too. @here and @everyone kind of drive me crazy on discord.

p00f · 2023-10-19T14:32:39Z

src/hinter/cwd_aware.rs

+use nu_ansi_term::{Color, Style};
+
+/// A hinter that uses the completions or the history to show a hint to the user
+/// NOTE: Only use this with `SqliteBackedHistory`!


@sholderbach How does this look?

sholderbach · 2023-10-19T17:24:10Z

Briefly, I don't think just documenting it is a good approach for the public API.

With a FileBackedHistory things can be compiled, the builder executes and then things panic on the first keypress.

Panicking patch

cargo run --features sqlite --example cwd_aware_hinter

diff --git a/examples/cwd_aware_hinter.rs b/examples/cwd_aware_hinter.rs
index 17aa7dd..35f9c2f 100644
--- a/examples/cwd_aware_hinter.rs
+++ b/examples/cwd_aware_hinter.rs
@@ -49,16 +49,14 @@ fn main() -> io::Result<()> {
         #[allow(deprecated)]
         let home_dir = std::env::home_dir().unwrap();
 
-        let history = create_filled_example_history(
-            &home_dir.to_string_lossy().to_string(),
-            &orig_dir.to_string_lossy().to_string(),
-        );
+        let history = reedline::FileBackedHistory::new(100); 
+
 
         let mut line_editor = Reedline::create()
             .with_hinter(Box::new(
                 CwdAwareHinter::default().with_style(Style::new().italic().fg(Color::Yellow)),
             ))
-            .with_history(history);
+            .with_history(Box::new(history));
 
         let prompt = DefaultPrompt::default();

Either we replace the current .expect()s in a way we can implement fall backs through recoverable error handling or we block at construction in .with_hinter/.with_history and provide a recoverable error there.

fallback/local error handling

As History::search already returns a result turning the expect here into a fall-back to a simpler query would be an option.
https://github.com/p00f/reedline/blob/00c59e4ca2762d5a12579b68de5dd505318fbfc4/src/hinter/cwd_aware.rs#L31

check at construction time

You are right that trait objects are limited on its own, but for "RTTI we have at home" the cost of adding a method that advertises a particular capability doesn't sound too bad if it is only checked in the constructors.

P.S.: just to disagree with @fdncred I really like this feature as a default and would be ok with saving ourselves another config option (just strawmanning it in for API design purposes)

p00f · 2023-10-19T18:07:10Z

Sorry, it took me a while to realize how "just don't use it" sounds.

I went with the first option as the second one requires more thought. Thanks for the help!

p00f · 2023-10-19T18:21:07Z

re: config option, we can introduce this without an option on main, then add it if people want to disable it. There's some time before the next release.

p00f · 2023-10-19T19:08:03Z

@sholderbach i fixed the warning causing the CI failure, can you re-run it?

sholderbach · 2023-10-19T20:02:01Z

Thanks for implementing the fallback!

re: config option, we can introduce this without an option on main, then add it if people want to disable it. There's some time before the next release.

Agree, your nushell patch looked already pretty good.

p00f · 2023-10-19T20:09:58Z

sorry, I think this one should pass

p00f · 2023-10-19T20:15:22Z

should I squash?

sholderbach · 2023-10-19T20:51:15Z

No need for manual squashing, we happily use the Github button for that.

Thanks for improving the quality of the hints. Looking forward to using them in Nushell

p00f · 2023-10-19T20:51:30Z

How do I do the nushell PR? By uncommenting https://github.com/nushell/nushell/blob/d0dc6986dd14e7ea5443f3dc0eb5b101c5de5efc/Cargo.toml#L167 ?

sholderbach · 2023-10-19T20:53:02Z

Yup uncommenting that line and running cargo update -p reedline to guarantee you get the most recent version on that branch

p00f · 2023-10-19T20:56:03Z

Yeah - I was wondering if it was acceptable to commit into main, then I read the diff - it was uncommented in main when I started working 😄

add cwd aware hinter

3ce7488

towards nushell/nushell#8883

handle the case where get_current_dir returns Err

3e02a1e

WIP cwd aware hinter

51f05e6

- guard CwdAwareHinter with feature flag - remove references to fish from DefaultHinter as fish is cwd aware - add example

document that CwdAwareHinter is only compatible with sqlite history

00c59e4

p00f commented Oct 19, 2023

View reviewed changes

p00f added 2 commits October 20, 2023 00:26

handle non-sqlite history

05b9d5e

handle no sqlite feature in example

c9766b1

sholderbach approved these changes Oct 19, 2023

View reviewed changes

fix warnings

94fa7d6

sholderbach merged commit 973dbb5 into nushell:main Oct 19, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cwd aware hinter #647

add cwd aware hinter #647

p00f commented Oct 17, 2023

codecov bot commented Oct 17, 2023 •

edited

Loading

p00f commented Oct 17, 2023

fdncred commented Oct 17, 2023 •

edited

Loading

p00f commented Oct 17, 2023

fdncred commented Oct 17, 2023

p00f commented Oct 17, 2023

fdncred commented Oct 17, 2023

p00f commented Oct 17, 2023

p00f commented Oct 17, 2023

p00f commented Oct 17, 2023

sholderbach commented Oct 18, 2023

p00f commented Oct 18, 2023

p00f commented Oct 18, 2023

p00f commented Oct 18, 2023

p00f commented Oct 18, 2023

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023

fdncred commented Oct 19, 2023

p00f commented Oct 19, 2023

fdncred commented Oct 19, 2023

p00f Oct 19, 2023

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023

p00f commented Oct 19, 2023

p00f commented Oct 19, 2023

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023

p00f commented Oct 19, 2023

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023 •

edited

Loading

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023

add cwd aware hinter #647

add cwd aware hinter #647

Conversation

p00f commented Oct 17, 2023

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

p00f commented Oct 17, 2023

fdncred commented Oct 17, 2023 • edited Loading

p00f commented Oct 17, 2023

fdncred commented Oct 17, 2023

p00f commented Oct 17, 2023

fdncred commented Oct 17, 2023

p00f commented Oct 17, 2023

p00f commented Oct 17, 2023

p00f commented Oct 17, 2023

sholderbach commented Oct 18, 2023

p00f commented Oct 18, 2023

p00f commented Oct 18, 2023

p00f commented Oct 18, 2023

p00f commented Oct 18, 2023

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023

fdncred commented Oct 19, 2023

p00f commented Oct 19, 2023

fdncred commented Oct 19, 2023

p00f Oct 19, 2023

Choose a reason for hiding this comment

sholderbach commented Oct 19, 2023

fallback/local error handling

check at construction time

p00f commented Oct 19, 2023

p00f commented Oct 19, 2023

p00f commented Oct 19, 2023

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023

p00f commented Oct 19, 2023

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023 • edited Loading

sholderbach commented Oct 19, 2023

p00f commented Oct 19, 2023

codecov bot commented Oct 17, 2023 •

edited

Loading

fdncred commented Oct 17, 2023 •

edited

Loading

p00f commented Oct 19, 2023 •

edited

Loading