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

Add explicit flag to read from standard input #242

Merged
merged 7 commits into from Jul 13, 2022

Conversation

pkazmier
Copy link
Contributor

Fixes bug introduced in #240 where interactive use of zk new results
in the command sitting indefinitely for user to supply user input.

This change introduces an explicit flag to new to indicate standard
input should be read for the content of the note. The flag is called
-i on --input.

The flag name is somewhat confusing as there is a --no-input flag at
the global level. I propose that we change that flag name to
--no-prompt as it more accurately describes the option. To maintain
backwards compatibility, we define --no-input as an alias.

Fixes bug introduced in zk-org#240 where interactive use of `zk new` results
in the command sitting indefinitely for user to supply user input.

This change introduces an explicit flag to `new` to indicate standard
input should be read for the content of the note.  The flag is called
`-i` on `--input`.

The flag name is somewhat confusing as there is a `--no-input` flag at
the global level.  I propose that we change that flag name to
`--no-prompt` as it more accurately describes the option.  To maintain
backwards compatibility, we define `--no-input` as an alias.
Apparently, kong `aliases` are only for commands. It did not work as I
had expected. At this point, `--no-input` has been removed. If we want
to keep it for backwards compatibility, I suppose we could add a new
hidden command line arg called `--no-input`. I defer to you for your
thoughts.
@pkazmier
Copy link
Contributor Author

pkazmier commented Jul 12, 2022

Apparently, kong aliases are only for commands. Providing a simple
backwards compatible --no-input option did not work as I had
expected. At this point, --no-input has been simply renamed to
--no-prompt. If we want to keep it for backwards compatibility,
I suppose we could add a new hidden command line arg called
--no-input. I defer to you for your thoughts.

@mickael-menu
Copy link
Collaborator

mickael-menu commented Jul 12, 2022

Thanks for tackling this!

I'd like to keep the --no-input name because it's a pretty standard flag for this. However, if you rename -i to --interactive it removes the issue while also being consistent with the --interactive flag of zk list.

This interactive mode for zk new is an interesting (albeit unintended!) new feature. But I think we also need to revert to the ReadStdinPipe() mode when --interactive is not set.

I'll need to check if there's a way to read all available stdin input without blocking until Ctrl-D, that would be a nicer behavior when --interactive is not set.

By the way, did you change anything in the SVG screencast? It appears as changed in the "Files changed" tab: docs/assets/media/list-format.svg

@pkazmier
Copy link
Contributor Author

I'd like to keep the --no-input name because it's a pretty standard flag for this. However, if you rename -i to --interactive it removes the issue while also being consistent with the --interactive flag of zk list.

Yes, interactive makes more sense. Not sure what I was thinking tbh.

This interactive mode for zk new is an interesting (albeit unintended!) new feature. But I think we also need to revert to the ReadStdinPipe() mode when --interactive is not set.

I don't think it's common for a CLI tool to selectively choose when to read from stdin based on whether the data is coming from a pipe or not, so I'm not sure we need to, but in the spirit of backwards compatibility, it certainly doesn't hurt.

I'll need to check if there's a way to read all available stdin input without blocking until Ctrl-D, that would be a nicer behavior when --interactive is not set.

I don't believe there is any standard means to do so.

By the way, did you change anything in the SVG screencast? It appears as changed in the "Files changed" tab: docs/assets/media/list-format.svg

Yes, in the screen shot, there was a --no-input that I changed to --no-prompt. I'll revert all those and make the changes you suggest above tonight or tomorrow morning if you want.

@mickael-menu
Copy link
Collaborator

Thank you, that would be great 🙏

This option instructs the `new` command to read the initial contents of
the note from standard input.  This allows the use to pipe or use shell
redirection with note creation:

```sh
$ zk new --interactive < file.txt
$ echo "Initial content" | zk new --interactive
```
@pkazmier
Copy link
Contributor Author

With the last commits, I've undone the prior changes and only added the -i / --interactive flag which controls whether or not zk new reads from standard input. I also added a section in note-creation.md to reflect this new flag.

Upon thinking about our earlier discussion, I really believe that zk new should only read from standard input, for either shell redirection or shell piping, if told to do so explicitly via the --interactive flag. In my opinion, a unix tool should not implicitly read from standard input sometimes, but other times explicitly—it's inconsistent and not the unix way.

This becomes even more apparent when one reads note-creation.md (the one in this commit). While that last section, my first question is why do I sometimes need to use --interactive to read from stdin (redirection case) while other times I don't (pipe case). As a hardcore unix user, stdin is stdin regardless of where it comes from. It shouldn't be conditional.

It also simplifies the code without needing to add back ReadStdinPipe. Okay, I've pleaded my case the best I can, will wait for your feedback. 😄

Finally, regarding tesh. I had a hard time finding this tool, but even once installed, it still failed to run. My version of tesh only seems to take one positional argument—not two. Are we using the same tool and/or version? Any help would be appreciated:

$ make tesh   
CGO_ENABLED=1 go build -tags "fts5" -ldflags "-X=main.Version=`git describe --tags --match v[0-9]* 2> /dev/null` -X=main.Build=`git rev-parse --short HEAD`" 
usage: tesh [-h] [--cd some/directory] [--setenv var=value] [--cfg arg] [--log arg] [--ignore-jenkins] [--wrapper arg] [--keep] [teshfile]
tesh: error: unrecognized arguments: tests/fixtures
make: *** [tesh] Error 2

$ tesh -h          
usage: tesh [-h] [--cd some/directory] [--setenv var=value] [--cfg arg] [--log arg] [--ignore-jenkins] [--wrapper arg] [--keep] [teshfile]

tesh -- testing shell

optional arguments:
  -h, --help           show this help message and exit

Options:
  teshfile             Name of teshfile, stdin if omitted
  --cd some/directory  ask tesh to switch the working directory before launching the tests
  --setenv var=value   set a specific environment variable
  --cfg arg            add parameter --cfg=arg to each command line
  --log arg            add parameter --log=arg to each command line
  --ignore-jenkins     ignore all cruft generated on SimGrid continuous integration servers
  --wrapper arg        Run each command in the provided wrapper (eg valgrind)
  --keep               Keep the obtained output when it does not match the expected one

$ brew info simgrid
simgrid: stable 3.31 (bottled)

@pkazmier
Copy link
Contributor Author

Re: tesh, I just realized it was your tool. I should have look at the GitHub workflows earlier. Doh!

@pkazmier
Copy link
Contributor Author

pkazmier commented Jul 13, 2022

Okay, in the hopes you agree with my prior comment and to help persuade you further, I've updated all the tesh tests where content was piped into zk new to use the new zk new --interactive flag. In addition, I've also update note-creation.md to reflect that any use of standard input, pipe or redirection, is done via the --interactive flag.

@pkazmier
Copy link
Contributor Author

Oh, and to further sweeten the pot, I also updated the screencasts where you pipe content into zk new to use the zk new --interactive instead. 😄

@mickael-menu mickael-menu linked an issue Jul 13, 2022 that may be closed by this pull request
@mickael-menu
Copy link
Collaborator

Upon thinking about our earlier discussion, I really believe that zk new should only read from standard input, for either shell redirection or shell piping, if told to do so explicitly via the --interactive flag. In my opinion, a unix tool should not implicitly read from standard input sometimes, but other times explicitly—it's inconsistent and not the unix way.

You convinced me, that's a sound argument.

Re: tesh, I just realized it was your tool. I should have look at the GitHub workflows earlier. Doh!

Ha yes, and it's a pretty experimental tool. I thought go-cmdtest was interesting but needed something a bit more flexible.

@mickael-menu mickael-menu merged commit a6e5225 into zk-org:main Jul 13, 2022
@pkazmier pkazmier deleted the fix/pr-240 branch July 13, 2022 12:37
@pkazmier
Copy link
Contributor Author

Upon thinking about our earlier discussion, I really believe that zk new should only read from standard input, for either shell redirection or shell piping, if told to do so explicitly via the --interactive flag. In my opinion, a unix tool should not implicitly read from standard input sometimes, but other times explicitly—it's inconsistent and not the unix way.

You convinced me, that's a sound argument.

Fantastic!

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.

zk new not working anymore
2 participants