Skip to content

Restore "Fix move to line start in multi-line history entries"#1078

Open
ymcx wants to merge 6 commits into
nushell:mainfrom
ymcx:fix-history-completion
Open

Restore "Fix move to line start in multi-line history entries"#1078
ymcx wants to merge 6 commits into
nushell:mainfrom
ymcx:fix-history-completion

Conversation

@ymcx
Copy link
Copy Markdown

@ymcx ymcx commented May 16, 2026

Closes #1075

This PR restores the changes reverted in #704 and originally submitted in #584.

The issue I was personally having with this reversion is that if you try to perform completion on a line from the history, it doesn't work correctly: if you for example type 'l' and press enter and then scroll up to the last line using the arrow up key and select a completion suggestion by pressing tab and enter, the appended completion will be erased upon the pressing of any key.

The original PR was reverted due to the added unit tests working incorrectly. This is due to the prompt being repainted after executing:
-> reedline.handle_event(&prompt, move_to_line_start_event.clone())
-> reedline.handle_editor_event(prompt, event)
-> reedline.submit_buffer(prompt)
-> reedline.repaint(prompt)

I fixed this issue by adding the #[cfg(not(test))] attribute on top of the last reedline.repaint function call, so that it is not invoked when running unit tests.

I also decided to rewrite the restored unit tests using #[test] rather than #[rstest] because of #704 (comment) talking about getting rid of rstest altogether. It seems like Reedline still makes use of rstest and as such I'm not sure if this rewrite was necessary.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 16, 2026

I've fine with replacing rstest altogether here, if you think it'll help, but we just need to place some comments so that it doesn't come back in another PR explaining why it's been replaced and not to re-add it. I'm not sure myself if porting rstests to tests is required but I'm up for it. I'm also up for leaving them if you think that's right too.

Looks like a pretty simple fix. We just need to get the ci green.

Thanks for working on this!

@ymcx
Copy link
Copy Markdown
Author

ymcx commented May 16, 2026

I've fine with replacing rstest altogether here, if you think it'll help, but we just need to place some comments so that it doesn't come back in another PR explaining why it's been replaced and not to re-add it. I'm not sure myself if porting rstests to tests is required but I'm up for it. I'm also up for leaving them if you think that's right too.

I'm fine with rstest, in fact, I'm not even sure what rstests are (lol). It's just that I noticed that there were no rstest attributes in the current version of the file and judging by the aforementioned comment regarding the refactor, I figured I'd just reintroduce the removed rstests as normal rust tests, since that's what I'm most comfortable with. All in all, I don't have a preference on this.

If I'm feeling like it, I'll probably refactor all the remaining rstests into normal tests and submit it as another PR. Sounds like they are preferred over rstest, but I'll probably need to do some quick research first on why that is.

Looks like a pretty simple fix. We just need to get the ci green.

Whoops, didn't even notice they were failing. I did run both cargo fmt and cargo clippy before submitting, but seems like I'm missing some switches/arguments?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 16, 2026

ok, sounds good.

@ymcx
Copy link
Copy Markdown
Author

ymcx commented May 16, 2026

The CI workflows should pass now. I had merged the main branch into this PR in commit e7de4e8 using the GitHub web interface and didn't do a git pull on my local instance and as such there were no errors on my machine.

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.

Appended completion is erased from line_buffer when pressing any key

2 participants