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

Add history exclusion configuration #518

Closed
wants to merge 3 commits into from

Conversation

rgwood
Copy link
Contributor

@rgwood rgwood commented Nov 24, 2022

A rough draft PR to solve #516, by letting users configure which command lines get excluded from history. For example, users can configure reedline so that command lines starting with a space are not saved.

@sholderbach Let me know what you think, happy to revise this.

@sholderbach
Copy link
Member

Doing the string match internally is certainly the minimally invasive change. How will this interact with the data amending as done for the sqlite history?

@rgwood
Copy link
Contributor Author

rgwood commented Nov 24, 2022

@sholderbach I don't think there's any special handling for SQLite history needed, because all the logic is happening in engine.rs. Perhaps I've missed something?

image

@rgwood rgwood marked this pull request as ready for review November 24, 2022 16:52
@sholderbach
Copy link
Member

Starting the session without a normal command being run crashes, as soon as I try to add an entry that is skipped (with --features sqlite):
image

I think it tries to modify the last entry (and passes if it can mess with the last succesfully stored one)

@rgwood
Copy link
Contributor Author

rgwood commented Nov 24, 2022

Oops! Will take a look and try to add some tests, thanks.

@rgwood
Copy link
Contributor Author

rgwood commented Nov 24, 2022

Hm. Y'know, I'm not sure whether that's a bug. A case could be made that the demo is intentionally not handling that edge case (where the very first command is ignored) well:

.expect("todo: error handling");

Do you have a suggested way forward?

@sholderbach
Copy link
Member

I think this rather indicates even with ignoring error handling that https://docs.rs/reedline/latest/reedline/struct.Reedline.html#method.update_last_command_context would try to modify the last remembered entry.

@rgwood
Copy link
Contributor Author

rgwood commented Nov 25, 2022

Right. I don't see the problem but I'm probably missing something.

edit: Oh, I think I get it now. I guess we might modify the wrong entry. :(

@rgwood
Copy link
Contributor Author

rgwood commented Dec 2, 2022

current status: I'm not really sure what the best way to implement this is. Might need to start storing the last command in memory instead of always looking it up in the DB. I've run out of steam to work on this for now but might come back to it another time; if anyone else wants to pick it up feel free.

@rgwood rgwood marked this pull request as draft December 2, 2022 01:37
@samlich samlich mentioned this pull request Apr 6, 2023
@sholderbach
Copy link
Member

Closed by #566

@sholderbach sholderbach closed this May 3, 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

2 participants