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

Added rustyline support for arrow keys + more #151

Merged
merged 7 commits into from
Mar 18, 2023
Merged

Added rustyline support for arrow keys + more #151

merged 7 commits into from
Mar 18, 2023

Conversation

TheRustyPickle
Copy link
Contributor

@TheRustyPickle TheRustyPickle commented Mar 8, 2023

What was added

  • Arrow up and down key to cycle history
  • Additional support of the left and right arrow keys to control the cursor, CTRL + C, and more. Check at rustyline

Changed

  • rustyline currently handles all input events instead of the previous stdin

Up for feedback

  • Whether to remove unused functions spawn_stdinput_handler and read_input
  • Change line_handling.rs or line_handler to something more concise?
  • The working directory is being used to save rustyline history. Suggestion for a concrete fixed path?
  • rustyline can potentially support autocompletion similar to a shell (still unsure how). Create another issue for it?

Related issues: #20 #142. Looking forward to hearing your thoughts, @24seconds! Also, let me know if I can improve the pr message in some way so you can understand more easily.

@24seconds 24seconds added the enhancement New feature or request label Mar 8, 2023
@24seconds 24seconds self-requested a review March 8, 2023 23:07
@24seconds
Copy link
Owner

wow thank you for the quick action! I will take a look later (Friday night ~ late night KST 🙇 )

@24seconds
Copy link
Owner

Sorry. Today was busy day. Let me review this until tomorrow mid afternoon 🙇

@24seconds 24seconds linked an issue Mar 11, 2023 that may be closed by this pull request
@24seconds
Copy link
Owner

[Update]
I was busy today also due to unexpected accident. Let me finish the review until weekend KST.

Copy link
Owner

@24seconds 24seconds left a comment

Choose a reason for hiding this comment

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

Thank you for the work! I left some nit comments.

src/line_handling.rs Outdated Show resolved Hide resolved
src/line_handling.rs Outdated Show resolved Hide resolved
src/line_handling.rs Outdated Show resolved Hide resolved
src/line_handling.rs Outdated Show resolved Hide resolved
src/line_handling.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@24seconds
Copy link
Owner

  • Whether to remove unused functions spawn_stdinput_handler and read_input
  • Change line_handling.rs or line_handler to something more concise?
  • The working directory is being used to save rustyline history. Suggestion for a concrete fixed path?
  • rustyline can potentially support autocompletion similar to a shell (still unsure how). Create another issue for it?
  1. Good. Let's remove it
  2. I left comment about naming. In my opinion, it would be nice to include input keyword in naming.
  3. I suggest in memory history. I also left comment. Please take a look.
  4. Oh! yes! It would be nice to work in another issue if it is complicated. If the code is not that long - under 10 lines (?), then you can do it in this pr.

Thank you @TheRustyPickle

@TheRustyPickle
Copy link
Contributor Author

Thank you, @24seconds. Just wanted to confirm this for point 1. If I remove read_input, I must also remove a test named test_read_command. I will be working on the rest of the points that were made.

@24seconds
Copy link
Owner

Thank you, @24seconds. Just wanted to confirm this for point 1. If I remove read_input, I must also remove a test named test_read_command. I will be working on the rest of the points that were made.

Yes, please remove. Btw, can we write a test case for newly introduced code @TheRustyPickle ?

@TheRustyPickle
Copy link
Contributor Author

Yes, sure. Will work on this as well.

@TheRustyPickle
Copy link
Contributor Author

Hello @24seconds, I couldn't figure out how to add tests for the rustyline code part. Also check the issue here about it. Hope the rest of the code is alright. Thanks for your time!

@24seconds
Copy link
Owner

Got it. Let me take a look today!

Copy link
Owner

@24seconds 24seconds left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much for the work! I enjoyed new feature. Thank you!

src/line_handler.rs Outdated Show resolved Hide resolved
@24seconds
Copy link
Owner

Thank you @TheRustyPickle !! Because of your recent contributions rust-cli-pomodoro app has improved a lot!

@24seconds 24seconds merged commit c393adc into 24seconds:main Mar 18, 2023
@24seconds
Copy link
Owner

I will update pakcage version in cargo and write release note on Sunday

@TheRustyPickle
Copy link
Contributor Author

Thank you for being so cooperative and understanding throughout the process, @24seconds! 🙏

@TheRustyPickle TheRustyPickle deleted the rustyline-impl branch March 18, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: arrow up key shows previous command
2 participants