Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Remove Readline usage #55

Merged
merged 13 commits into from
May 11, 2017
Merged

Remove Readline usage #55

merged 13 commits into from
May 11, 2017

Conversation

coryb
Copy link
Collaborator

@coryb coryb commented May 8, 2017

Due to complications and inconsistencies with Readline (that prevent autoplay from working reliably) I have removed Readline from the codebase. For the simple cases of Confirm and Input I have replaced with with bufio.Scanner. For the cases where we need to read a rune at at time I have implemented terminal.RuneReader with specializations for most Unix flavors along with Windows. I have tested it on Windows 10, Linux and OSX and it seems to work well.

Future refactoring:

  • fold the OnChange functions into the Prompts. I left them as-is to reduce the changes.
  • Add Help to Password prompts. This should just work now that I no longer have to fight against Readline.

@coryb coryb requested a review from AlecAivazis May 8, 2017 18:31
@AlecAivazis
Copy link
Owner

Works like a charm on my osx laptop! I'll try to pull this down and test it out tomorrow when I have access to a windows laptop. Minor point, it seems like ctrl+d causing a type casting panic since the prompt returns nil. My gut says that ctrl+d and ctrl+c should behave the same - thoughts?

@coryb
Copy link
Collaborator Author

coryb commented May 9, 2017

Cool, I have not tried ctrl-d. I will try to get that working.

@coryb
Copy link
Collaborator Author

coryb commented May 9, 2017

I just tried a few of the tests with ctrl-d and could not reproduce a panic, which tests did you see the issue with?

@AlecAivazis
Copy link
Owner

Ah, seems the behavior is inconsistent. tests/input.go panics with ctrl+d but tests/select.go does not respond at all.

@coryb
Copy link
Collaborator Author

coryb commented May 9, 2017

Okay, cool I will get this fixed. I tried tests/confirm.go which worked but prompt answer is misaligned.

@coryb
Copy link
Collaborator Author

coryb commented May 9, 2017

Fixed the panic and the alignment issues on ctrl-d input. Fixed ctrl-c and ctrl-d handling for Password, MultiSelect and Select. the bufio.Scanner on windows apparently does not handle ctrl-d directly, and ctrl-c will just terminate the program for Input and Confirm. We can look at polishing that up later, but I think it is good for now.

@AlecAivazis
Copy link
Owner

Just verified that the behavior is working as expected on Windows 10 for all of the tests! We're really damn close to finally putting this to rest.

However, there seems to be a small issue with the output on windows - it seems like there is an extra space in the templates that causes the question to be one space to the right the first time it renders. I'll try to get a screen shot from my windows laptop but if you run tests/selectThenInput.go you'll see that the ? What is your name? is not lined up with the ? Choose your color:

@coryb
Copy link
Collaborator Author

coryb commented May 9, 2017

Hey, I Just fixed the spurious space ahead of the ' ?' character. I also was annoyed about the inconsistency between ctrl-d on Input vs Password, so I added a ReadLine routine to the RuneReader object which can be used for both. This removes the use of bufio.Scanner. It seems to work now consistently.

@coryb
Copy link
Collaborator Author

coryb commented May 9, 2017

Ughh, one more slight bump, when I use default value for Input is not writing the Answer back correctly, trying to figure out what is going on now.

@coryb
Copy link
Collaborator Author

coryb commented May 9, 2017

Okay, this is not a new issue, the template has:

{{- if .Answer}}
  {{- color "cyan"}}{{.Answer}}{{color "reset"}}{{"\n"}}

So if the default answer is "" then it evaluates to false and the template shows the prompt again. I will try to think about a better way to test if we are supposed to print the Answer. I think this is a subsequent PR so as not to pollute this one further.

@coryb
Copy link
Collaborator Author

coryb commented May 10, 2017

I think this PR is good to go from my end. I don't have any further refactoring on it planned unless you want some changes to it.

@coryb
Copy link
Collaborator Author

coryb commented May 10, 2017

Whoops, forgot to mention that this PR will fix #52

@AlecAivazis
Copy link
Owner

Awesome! Thanks for fixing that so quickly. And thanks for finding that bug with .Answer. Unfortunately, I do not have access to the windows laptop at the moment so I can't verify that it works there, but will check it as soon as I can and get this merged in.

@@ -22,8 +20,8 @@ type Question struct {
// Prompt is the primary interface for the objects that can take user input
// and return a string value.
type Prompt interface {
Prompt(*readline.Instance) (interface{}, error)
Cleanup(*readline.Instance, interface{}) error
Prompt() (interface{}, error)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is probably my favorite part of this whole PR - Thankyou so much for removing readline! While having to support individual systems like this is a bit scary, we'll handle the obscure edge cases over time. Overall this is yet another huge improvement

@AlecAivazis
Copy link
Owner

Okay I just checked this out on a windows machine and everything seemed to work perfectly except that ctrl+d submits the current selection in Select and MultiSelect but we've already noted that. I'm going to merge this

@AlecAivazis AlecAivazis merged commit 92d1133 into AlecAivazis:master May 11, 2017
@coryb
Copy link
Collaborator Author

coryb commented May 11, 2017

I am confused, what behavior do you expect for ctrl-d on Select and MultiSelect?
It is end-of-transmission signal, I would expect to the Prompt to just accept whatever is selected and return those results.

@AlecAivazis
Copy link
Owner

Yea I'm still thinking about that, I guess I'm used to ctrl+d and ctrl+c behaving the same (ie as an interrupt) but i'm not sure what the right convention is. I think we should just leave it as it is for now and we'll worry about it later

@coryb
Copy link
Collaborator Author

coryb commented May 11, 2017

Okay, I see. Well ctrl-d is effectively an EOF. My personal opinion is that it would be pretty weird for a library to treat an EOF as an error.

@AlecAivazis
Copy link
Owner

AlecAivazis commented May 11, 2017

Yep, I agree. Leaving it as it is is what I'm in favor of too

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants