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

Provide a basic default implementation of History #209

Merged
merged 6 commits into from Sep 1, 2023
Merged

Provide a basic default implementation of History #209

merged 6 commits into from Sep 1, 2023

Conversation

Garbaz
Copy link
Contributor

@Garbaz Garbaz commented Jul 13, 2022

As it stands, if one wants to have an input with history, one has to define a custom struct and implement the History trait for it, which is quite some boilerplate code (See examples/history.rs). Since in most simple applications, one probably doesn't need any custom history logic (limited size / no duplicates / etc.), simply providing a default implementation for Vec and VecDeque seems sensible to me.

The VecDeque implementation is very straightforward and equivalent to the provided example, except without limiting the size. For the Vec implementation we have to index from the back, which requires checking in advance that the pos is in bounds, since otherwise we end up subtracting a usize below zero, which explodes.

@pksunkara pksunkara added this to the 0.11.0 milestone Apr 11, 2023
src/history.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Gordon01 Gordon01 left a comment

Choose a reason for hiding this comment

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

A history_infinite() method can be a good alias for VecDeque

@Garbaz
Copy link
Contributor Author

Garbaz commented Aug 30, 2023

A history_infinite() method can be a good alias for VecDeque

Unless I'm misunderstanding what you mean here, as it stands, I don't think this is possible, since the Input struct only contains an Option<&'a mut dyn History<T>>, so the history has to be owned outside somewhere and a &mut is handed in with Input::history_with. A method Input::history_infinite that internally creates a VecDeque has nowhere to store it with sufficient lifetime.

@@ -9,6 +9,10 @@ fn main() {

let mut history = MyHistory::default();

/// We can also use `Vec` or `VecDeque` directly for a simple infinite history.
// let mut history = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this line and update the comment above this line accordingly?

@Gordon01
Copy link
Contributor

There is a way to implement a history_infinite() method with minor changes to API and preserving backward compatibility in history_with() Gordon01#3

Also dialoguer can be shipped with a few algorithms of history management like dedup.

@Garbaz what do you think about it ?

@Garbaz
Copy link
Contributor Author

Garbaz commented Aug 31, 2023

There is a way to implement a history_infinite() method with minor changes to API and preserving backward compatibility in history_with() Gordon01#3

Also dialoguer can be shipped with a few algorithms of history management like dedup.

@Garbaz what do you think about it ?

I have implemented a very similar approach, just slightly more general, with history either being owned by the Input value or being borrowed. But the problem is that the intended way to use Input, as far as I can tell from the examples, is to construct it transiently for e.g. each loop iteration instead of having a mutable persistent bound value. This usage is not compatible with persistent owned history.

So, given how the API is set up, I think the best option is to leave the Input implementation as.


I do like the idea of having some basic history features included. So instead of the implementation of History for VecDeque, I've now added a BasicHistory, which by default is just an infinite history with duplicates (i.e. identical to the VecDeque implementation), but additionally allows for setting a limit on the number of entries and enabling automatic removing of duplicates.

@Garbaz Garbaz changed the title Added implementations of History for Vec and VecDeque Provide a basic default implementation of History Aug 31, 2023
@Gordon01
Copy link
Contributor

Great, I like it. Thank you!

We can make BasicHistory export/importable to be able to store history between launches.

@pksunkara pksunkara merged commit c4c00f4 into console-rs:master Sep 1, 2023
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.

None yet

3 participants