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

CLI wonky with readline fix #420

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

KenP97
Copy link

@KenP97 KenP97 commented May 16, 2022

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.

client/repl/client.go Outdated Show resolved Hide resolved
@daveyarwood
Copy link
Member

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
Copy link
Author

KenP97 commented May 17, 2022

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
Copy link
Member

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
Copy link
Member

daveyarwood commented May 18, 2022

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
Copy link
Author

KenP97 commented May 18, 2022

I think so ... they did something

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

@daveyarwood
Copy link
Member

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.

@daveyarwood daveyarwood mentioned this pull request Jul 27, 2024
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.

2 participants