Skip to content

Stop panicking in Current() #81

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Stop panicking in Current() #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ Golang package for dealing with consoles. Light on deps and a simple API.
## Modifying the current process

```go
current := console.Current()
current, err := console.Current()
if err != nil {
}
defer current.Reset()

if err := current.SetRaw(); err != nil {
Expand Down
10 changes: 4 additions & 6 deletions console.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,17 @@ type WinSize struct {
}

// Current returns the current process' console
func Current() (c Console) {
var err error
func Current() (c Console, err error) {
Copy link
Member

@crosbymichael crosbymichael Jan 12, 2024

Choose a reason for hiding this comment

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

Instead of renaming this, can we add an additional constructor that returns the error? Something like:

func GetCurrent() (Console, error)

Copy link
Member

@crosbymichael crosbymichael Jan 12, 2024

Choose a reason for hiding this comment

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

Then we shouldn't have to worry about the breaking change.

Copy link
Member

@thaJeztah thaJeztah Feb 5, 2024

Choose a reason for hiding this comment

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

👍 this code may be incorporated in various projects, and we should try to avoid breaking this signature (it may be complex to resolve otherwise). I don't have a good suggestion for the new function's name (perhaps someone else has?). Perhaps TryCurrentConsole() ?

That said, it looks like Current() is a very small wrapper around ConsoleFromFile(), so to some extent, users would already be able to work around this.

We should improve the GoDoc for Current() though!

  • Outline what it does (tries the standard-streams (STDERR, STDOUT, STDIN to get the console)
  • It panics when it fails to detect a console in any of those.
  • Point users to the alternative / equivalent using ConsoleFromFile

// Usually all three streams (stdin, stdout, and stderr)
// are open to the same console, but some might be redirected,
// so try all three.
for _, s := range []*os.File{os.Stderr, os.Stdout, os.Stdin} {
if c, err = ConsoleFromFile(s); err == nil {
return c
return c, nil
}
}
// One of the std streams should always be a console
// for the design of this function.
panic(err)

return nil, err
}

// ConsoleFromFile returns a console using the provided file
Expand Down