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

nvm.sh: added nvm_stdout_is_terminal() #2007

Merged
merged 1 commit into from Mar 3, 2019

Conversation

Projects
None yet
2 participants
@jcsahnwaldt
Copy link
Contributor

jcsahnwaldt commented Feb 23, 2019

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 24, 2019

You've got good tests! I didn't know I had to add nvm_is_terminal to the unload section. Fixed. I just added it at the end. Hope that's OK.

@ljharb
Copy link
Collaborator

ljharb left a comment

We'd need some unit tests for nvm_is_terminal; however when I try this locally it doesn't seem to detect that it's in a pipe.

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 24, 2019

I'll try to add unit tests tomorrow.

I ran a few simple tests with nvm_is_terminal on my machine (macOS 10.13.6, bash 3.2.57), and it seems to work. For example, I added these lines at the start of nvm_ls():

if nvm_is_terminal; then
  echo term
else
  echo no term
fi

When I call the function with / without a pipe, the results look OK:

> nvm_ls
term
N/A
> nvm_ls | cat
no term
N/A

(I ran . ./nvm.sh without proper configuration, so the N/A is OK.)

Could you post more details about what's not working on your machine?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 24, 2019

I put the function in my shell, and piped something to it. (specifically, a function that echoed yes or no based on that -t test)

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 25, 2019

Testing the function in the terminal can be a bit delicate...

First, I'm not sure what you mean by "piped something to it", but it might be the wrong thing. The 1 in [ -t 1 ] stands for file descriptor 1, i.e. stdout. So the function nvm_is_terminal only tests whether stdout is a terminal and ignores stdin and stderr. That makes sense for our use case: As far as I can tell, no nvm command reads any data from stdin, they all just write stuff to stdout (and error messages to stderr, but they don't matter here). If you pipe something into the function, e.g. nvm_is_terminal </dev/null, you're replacing stdin, which doesn't affect the result of the function.

Shell syntax can also be tricky. For example, this command

$ nvm_is_terminal ; echo $?
0

has the expected result 0, because I'm calling nvm_is_terminal in a terminal. (As usual, shell truth is the opposite of C truth: 0 means success or true, all other numbers mean failure or false.)

If I want to check the result of nvm_is_terminal in a pipe, I might do this:

$ nvm_is_terminal ; echo $? | cat
0

Oops... Still 0. Is nvm_is_terminal broken? No, my syntax is wrong. The pipe | only affects echo, not the part before the ;. This syntax is correct:

$ (nvm_is_terminal ; echo $?) | cat
1

Now both commands are attached to the pipe, and I get the expected result 1 (meaning "not a terminal").

I'm pretty sure nvm_is_terminal is working correctly on my machine, but I may be missing something. Could you post the exact shell commands you're using?

P.S.: The -t test is documented on the man page for the test utility. ([...] is short for test ...).

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 25, 2019

nvm_is_terminal() { if [ -t 1 ]; then echo y; else echo n; fi } ; 
echo hi | nvm_is_terminal // i expect n, i get y
nvm_is_terminal // i get y as expected

@ljharb ljharb force-pushed the jcsahnwaldt:patch-2 branch 2 times, most recently from 619133a to da7de16 Feb 25, 2019

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 26, 2019

echo hi | nvm_is_terminal // i expect n, i get y

That's the stdin case I mentioned in my previous comment. y is the expected result, because the output of nvm_is_terminal still goes to a terminal in this case.

Try this instead:

$ nvm_is_terminal() { if [ -t 1 ]; then echo y; else echo n; fi }
$ nvm_is_terminal | cat
n

nvm_is_terminal only checks whether the output (stdout) goes to a terminal. It doesn't care about the input (stdin). And that's the way it should be: We need to check whether we should modify the output of nvm ls etc. We don't care about any input.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 27, 2019

Thanks, that clarifies it. Perhaps a better name then would be nvm_outputs_to_terminal or something? Also, does this only check stdout - what about stderr? If stdout is redirected to a file, or to dev/null, is that "a terminal"?

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 27, 2019

It only checks stdout. (It would be easy to also check stderr, but I don't think that would make much sense. I think when people redirect stderr, they expect normal output not to be affected, so I guess the part of the code that decides how output will be formatted should not check stderr. For example, a common idiom to discard error messages is 2>/dev/null, and that should not affect what gets written to stdout.)

If stdout is redirected to a file or /dev/null, that would not be a terminal. The specification for the [ -t ] flag says: "True if ... file_descriptor is open and is associated with a terminal. False if ... file_descriptor is not open, or if it is open but is not associated with a terminal."

I like short names, so I chose nvm_is_terminal, but maybe that's too short. :-) Maybe nvm_stdout_is_terminal would be best? Clear, precise, not too long...?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 27, 2019

nvm_stdout_is_terminal seems clear enough that if i forget all about this PR and find it in a year, I'll still know what it means :-) let's go for it.

@jcsahnwaldt jcsahnwaldt force-pushed the jcsahnwaldt:patch-2 branch from da7de16 to 720292b Feb 27, 2019

@jcsahnwaldt jcsahnwaldt changed the title nvm.sh: added nvm_is_terminal() nvm.sh: added nvm_stdout_is_terminal() Feb 27, 2019

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 27, 2019

OK, I renamed the function to nvm_stdout_is_terminal (also in the unload section) and renamed this PR. I'll look at the unit tests next.

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 27, 2019

The nvm_stdout_is_terminal test passed in all shells. Nice. :-) Is there anything else I can do for this PR?

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 27, 2019

Added another little fix. (A failure in one of the tests didn't properly signal a failure, it just printed an error message.)

@jcsahnwaldt jcsahnwaldt force-pushed the jcsahnwaldt:patch-2 branch 2 times, most recently from dca23a7 to 1f6873b Feb 27, 2019

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 27, 2019

I found a way for the test with the pipe to transfer the return value of nvm_stdout_is_terminal back to the test engine. It's a bit cumbersome and brittle, but I think it's good enough.

I temporarily checked in a deliberately broken version of the test to see if it fails on travis-ci. I simply changed this line

(! nvm_stdout_is_terminal || echo "boo!") | read && die 'nvm_stdout_is_terminal should be false when stdout goes to a pipe'

to this (without the negation at the start):

(nvm_stdout_is_terminal || echo "boo!") | read && die 'nvm_stdout_is_terminal should be false when stdout goes to a pipe'

The results are here: It signals an error in bash and zsh (good), but success in dash and sh (bad).

I don't have dash, so I can't test it. I do have sh, and when I run the deliberately broken test locally in sh, it signals an error (as expected). So I don't know what's going on with dash and sh on travis-ci because I can't reproduce it. But I guess bash and zsh are much more relevant anyway, so I'd say the tests are good enough...

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 28, 2019

sh is usually bash on many machines; dash is something you'd have to install separately.

I'll take a look; it'd be ideal to make the test fail in dash as well, when expected. Can we modify the test so it's both testing true and false cases?

@jcsahnwaldt

This comment has been minimized.

Copy link
Contributor Author

jcsahnwaldt commented Feb 28, 2019

Oh, you're right, on my Mac sh is just bash with a different prompt. :-)

I just installed dash (via Homebrew) and ran the test by executing ./nvm_stdout_is_terminal in ~/.nvm/test/fast/Unit tests. Same result as for all other shells: When I break one of the tests by toggling the ! at the start, the test fails and the return value is 1 (I checked it by echo $?). I checked all 13 tests in dash. So it looks like travis-ci does something strange with dash and sh in this case.

The ./nvm_stdout_is_terminal test already checks true and false cases as much as possible, but we can't test the "real" true case on travis-ci, because the tests there are not attached to a terminal. I added a few tests that redirect stdout to /dev/tty, and nvm_stdout_is_terminal returns true in that case (also, as far as I can tell, on travis-ci - otherwise, the last three tests would fail).

Anyway, I wouldn't bother with the tests on travis-ci anymore. I mean, the function nvm_stdout_is_terminal() is extremely simple (just 40 characters long, 22 of which are the name). I'm sure it's correct. I ran the tests in all four shells on my machine, and they work as expected. They also work as expected on travis-ci in almost all cases. Also, the test file has 1805 characters, so that's an extremely good test code to production code ratio. ;-)

@ljharb ljharb force-pushed the jcsahnwaldt:patch-2 branch from 1f6873b to 2410215 Mar 3, 2019

@ljharb

ljharb approved these changes Mar 3, 2019

@ljharb ljharb merged commit 2410215 into creationix:master Mar 3, 2019

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jcsahnwaldt jcsahnwaldt deleted the jcsahnwaldt:patch-2 branch Mar 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.