CLI wonky with readline fix

KenP97 commented 2 years ago

Reference Issues/PRs

Addresses #405

What does this implement/fix? Explain your changes.

Changed the color library for client to support windows by using the "github.com/gookit/color" library
Changed the readline library for client to support windows and prevent background changes, replaced with "github.com/peterh/liner"
Added tempfile ".alda_history" in temp directory for handling previous inputs, and reverseSearch added.

TODO:

need to connect zerolog with the new liner library to handle Stderrs
need to test multiLine mode, and to understand how it works ... to make playing instruments easier.

daveyarwood commented 2 years ago

Hi @KenP97 , thanks so much for the PR! I'm busy with work this week, but I hope to review and test it in more depth soon.

Another quick comment: let's not include build files (e.g. .exe files) in the repo.

KenP97 commented 2 years ago

Another quick comment: let's not include build files (e.g. .exe files) in the repo.

I'll make sure to remember this.

Changed back to using the replHistoryFilepath

daveyarwood commented 2 years ago

I had a few minutes to test this branch tonight, and it looks like it's working overall, but I did observe a couple of issues:

1. Logging doesn't play nice with REPL prompt

As I suspected, not having that log.SetOutput(...) part does have the consequence that logging gets in the way of the prompt. For example, here is the output on this branch, running an Alda REPL with verbosity turned up:

$ client/bin/run -v2 repl
...
alda> May 17 22:14:24 INF repl/player_management.go:164 > Found player process. player={"Expiry":1652840616079,"ID":"jpg","Port":32973,"State":"ready"}
alda> piano: c
May 17 22:14:27 INF repl/server.go:331 > Request received. decodedRequest={"code":"piano: c","id":"6bd0888d-58f4-48b1-bb5f-5b04f3f28572","op":"eval-and-play","session":"124eab46-bbf2-4879-870f-c8a31c49381e"}
...
alda> c
May 17 22:14:30 INF repl/server.go:331 > Request received. decodedRequest={"code":"c","id":"86a9b27d-6673-4565-8db3-4564c83daa16","op":"eval-and-play","session":"124eab46-bbf2-4879-870f-c8a31c49381e"}
...
alda> c/e/g
May 17 22:14:32 INF repl/server.go:331 > Request received. decodedRequest={"code":"c/e/g","id":"5298911e-3492-4a06-85da-b6e87d4f0159","op":"eval-and-play","session":"124eab46-bbf2-4879-870f-c8a31c49381e"}
...
alda>
alda>
alda>
alda> May 17 22:14:34 INF system/process_management.go:511 > Spawned player process.

alda>

vs. the current release of Alda:

$ alda -v2 repl
...
May 17 22:14:41 INF system/process_management.go:511 > Spawned player process.
May 17 22:14:41 INF system/process_management.go:511 > Spawned player process.
May 17 22:14:41 INF repl/player_management.go:164 > Found player process. player={"Expiry":1652840423296,"ID":"xtf","Port":43375,"State":"ready"}
alda> piano: c
May 17 22:14:46 INF repl/server.go:331 > Request received. decodedRequest={"code":"piano: c","id":"d693cb0c-48b8-46d3-9bc9-60fa9ed08d71","op":"eval-and-play","session":"cadddad6-f7f0-4161-8acd-cb6205b4b3f7"}
...
alda> c/e/g
May 17 22:14:50 INF repl/server.go:331 > Request received. decodedRequest={"code":"c/e/g","id":"06711048-cd5a-4df3-b9a4-0edb407aca71","op":"eval-and-play","session":"cadddad6-f7f0-4161-8acd-cb6205b4b3f7"}
...
alda> c
May 17 22:14:54 INF repl/server.go:331 > Request received. decodedRequest={"code":"c","id":"67ed636e-b969-4563-bc43-bac98c3c829e","op":"eval-and-play","session":"cadddad6-f7f0-4161-8acd-cb6205b4b3f7"}
...
alda>
alda>

2. History not working as expected

It seems like the history file is being read correctly, but it isn't writing to the file. When I start the REPL, I can use the up/down arrows to go up through the history of lines that I've entered in the release Alda REPL. But when I enter new lines, they aren't included in the history file, nor are they in the history as I use the up/down arrows.

daveyarwood commented 2 years ago

Another potential issue: I see that you are using the Gookit color library directly in various files instead of using alda.io/client/color. alda.io/client/color is doing some important things like disabling color altogether if NO_COLOR is set or if Alda is running in an environment that doesn't support terminal colors, like some Windows environments. See the comment at the top of that file for more info.

If Gookit already handles the same things correctly, then it's fine to use Gookit directly, but otherwise, we should continue to use alda.io/client/color as a single interface for color, and ensure that it still works as desired when Aurora is switched out for Gookit.

EDIT: If I'm understanding this correctly, Gookit may actually make it so that we don't need to worry about whether the environment supports terminal colors, because Gookit works in a variety of environments, including Windows CMD. So we might be able to remove the isatty.IsTerminal(os.Stdout.Fd()) part (which is essentially checking for color support in the terminal). But we still need to make sure that NO_COLOR works.

EDIT 2: Whoa! I just tried NO_COLOR=true client/bin/run repl and it works out of the box! It looks like Gookit supports NO_COLOR. Nice!

It looks like we do still need alda.io/client/color for logging related reasons. We should have another look at Zerolog and see if maybe they've added support for NO_COLOR in the last year. And if there are no color-related issues in environments like Windows CMD. If so, then maybe we can get rid of alda.io/client/color altogether.

KenP97 commented 2 years ago

I think so ... they did something

log.Logger = log.Output(&zerolog.ConsoleWriter{Out: os.Stdout, NoColor: os.Getenv("NO_COLOR")})

daveyarwood commented 2 years ago

That's just an example of how we can disable color based on NO_COLOR, and we are currently doing that in color.go. It would be better if Zerolog did that check for us, but if they still don't, we can keep doing what we're doing in color.go.

alda-lang / alda