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

Check for io.Writer #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

cardil
Copy link

@cardil cardil commented Feb 28, 2023

Fixes #80

@cardil
Copy link
Author

cardil commented Mar 4, 2023

@mattn Can you review this?

@dolmen
Copy link
Contributor

dolmen commented Mar 15, 2023

@cardil You have a typo in filenames: writter.go -> writer.go (same for test file).

Please fix the typo and git commit --amend && git push -f.

return false
}

type filelike interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a type alias to avoid defining a real type:

Suggested change
type filelike interface {
type filelike = interface {

Copy link
Author

Choose a reason for hiding this comment

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

Definitely can do it. I'm just wondering why? The type is private, so what's the point?


func TestIsWriterTerminal(t *testing.T) {
// test for non-panic
isatty.IsWriterTerminal(os.Stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Log test result:

Suggested change
isatty.IsWriterTerminal(os.Stdout)
t.Log("os.Stdout:", isatty.IsWriterTerminal(os.Stdout))


func TestIsWriterCygwinTerminal(t *testing.T) {
// test for non-panic
isatty.IsWriterCygwinTerminal(os.Stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Log test result:

Suggested change
isatty.IsWriterCygwinTerminal(os.Stdout)
t.Log("os.Stdout:", isatty.IsWriterCygwinTerminal(os.Stdout))

@dolmen
Copy link
Contributor

dolmen commented Mar 15, 2023

I think it would be helpful to add the symetric support for io.Reader.

@cardil
Copy link
Author

cardil commented Mar 15, 2023

I think it would be helpful to add the symetric support for io.Reader.

I can file another PR for that

@dolmen
Copy link
Contributor

dolmen commented Mar 15, 2023

@cardil

  1. The filelike interface will be common to both.
  2. Both should be added at the same time in the API. io.Writer has no priority over io.Reader. (In fact I personally use go-isatty more to check os.Stdin than os.Stdout)

@cardil
Copy link
Author

cardil commented Mar 15, 2023

  1. The filelike interface will be common to both.

Yes, maybe. That doesn't mean we need to have that in single PR.

  1. Both should be added at the same time in the API. io.Writer has no priority over io.Reader. (In fact I personally use go-isatty more to check os.Stdin than os.Stdout)

There's no reason to add those at the same time? Why? It's a new API, that I'm proposing here, so why would be need to add additional features to this PR. I rather have separate, smaller PRs, that can ve reviewed independently.

@dolmen
Copy link
Contributor

dolmen commented Mar 22, 2023

My point is that new API for io.Writer and io.Reader are NOT independent. Consistency matters.

@dolmen
Copy link
Contributor

dolmen commented Mar 23, 2023

Anyway, I'm fully against this feature as I explained here: #80 (comment)

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.

Allow to check a io.Writer instead of raw file descriptor
2 participants