Restore "Fix move to line start in multi-line history entries"#1078
Conversation
|
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! |
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.
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? |
|
ok, sounds good. |
|
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. |
ymcx
force-pushed
the
fix-history-completion
branch
from
90de868 to
1cec7fa
Compare
I haven't really reviewed your approach for fixing the stdout spam issue, but I'm sure it's better than my initial approach. I did a hard reset back to the main branch, cherry-picked the restoration commit f6dc83e, the history line completion unit test commit 9967973 and merged all commits from upstream 1cec7fa. While at it, I rewrote the unit tests in the first commit as rstests and force-pushed all commits to make the history more clean. The problem now is I'm unable to pass any of the three unit tests (the two added back in the first commit and the one I wrote in the second commit). |
|
Maybe i can take a look later and see whats going on. |
|
I was able to fix the last unit test, currently I'm looking at fixing the rest of them and submitting the fixes here. |
|
Nice thanks! |
3 participants
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.