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

Support reading to stdout via builtin read - #4407

Merged
merged 7 commits into from Oct 10, 2017
Merged

Conversation

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Sep 16, 2017

Added an option to read to stdout via read -. While it may seem
useless at first blush, it lets you do things like include

mysql -p(read --silent) ...

Without needing to save to a local variable and then echo it back.
Kicks in when - is provided as the variable name to read to. This is
in keeping with the de facto syntax for reading/writing from/to
stdin/stdout instead of a file in, e.g., tar, cat, and other standard
unix utilities.

It doesn't make anything possible that wasn't previously doable with (read temp_var; echo $temp_var) but it doesn't interfere with anything either and makes fish that much easier to use. Willing to reconsider if there is friction against including this in 3.0.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Sep 16, 2017

Is it ok to use std::wcout directly or should I be using some internal API to write to the shell?

Loading

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Sep 16, 2017

Is there a convention that - means to output to stdout? I've seen it for stdin but can't remember seeing it for stdout.

Don't use wcout - who knows what that does, and also the wide character output functions are buggy.

Elsewhere in the shell we use wcs2string followed by write_loop with STDOUT_FILENO.

Loading

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Sep 17, 2017

Is there a convention that - means to output to stdout?

Yes, but it is very rare. The tar command is the only example that comes to mind. You would typically do tar -cf - ... to write the archive to stdout. The cat command does not use - to mean write to stdout so not sure what @mqudsi was thinking when he listed that as an example.

I have to say I'm ambivalent about this change. In part because read - is ambiguous. Does it mean read from stdin, what most people expect when they see - used that way, or write to stdout? But also because it doesn't make an uncommon pattern all that much easier. At least not enough to justify the feature in my opinion. How often have you seen scripts that read some input and immediately echo it into a pipeline? In my experience that pattern is quite rare.

A bigger objection is that the current implementation allows mixing var names and the magic - token (e.g., read x - y) which looks like a source of confusion and bugs.

If this feature is added I would request that - not be allowed if other var names are present. Obviously this also means changing the var name validation code to also only allow a single instance of -. Also, that appropriate unit tests be added.

Loading

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Sep 17, 2017

P.S., We really shouldn't be having this discussion in a change review. It belongs in an enhancement issue.

Loading

@zanchey
Copy link
Member

@zanchey zanchey commented Sep 17, 2017

Would it be better to allow read to take no arguments, and echo to standard output in that case?

Loading

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Sep 17, 2017

Would it be better to allow read to take no arguments, and echo to standard output in that case?

Yes, that would be much better from a UI perspective and simpler to implement correctly. I'm still unconvinced it's useful enough to implement but if done that way I'd shrug my shoulders and give a weak thumbs up since it is marginally useful and doesn't overly complicate the code or introduce hard to document behavior.

Loading

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Sep 17, 2017

Would it be better to allow read to take no arguments, and echo to standard output in that case?

Fwiw, that's what I originally presumed read did.

Loading

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Sep 17, 2017

Fwiw, that's what I originally presumed read did.

That would have been the reasonable thing to do but instead it simply discarded the input. See issue #4220.

Loading

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Sep 17, 2017

Also think about how this feature interacts with read --array regardless of the previous comments.

Loading

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 6, 2017

The branch has been updated per the discussion above. read, when called without any parameters, now reads directly to stdout. This resolves

  • mixing of reading to variables and stdout
  • multiple reads to stdout
  • stdout in array mode
  • what to insert as a delimeter if a custom delimiter is used
  • etc

Additionally, the documentation has been updated to reflect the changes to read. Feedback appreciated.

Loading

mqudsi added 7 commits Oct 10, 2017
Added an option to read to stdout via `read -`. While it may seem
useless at first blush, it lets you do things like include

    mysql -p(read --silent) ...

Without needing to save to a local variable and then echo it back.
Kicks in when `-` is provided as the variable name to read to. This is
in keeping with the de facto syntax for reading/writing from/to
stdin/stdout instead of a file in, e.g., tar, cat, and other standard
unix utilities.
No longer using `-` to indicate reading to stdout. Use lack of arguments
as stdout indicator. This prevents mixing of variables with stdout
reading and makes it clear that stdout may not be mixed with delimiters
or array mode.
@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 10, 2017

Fixed unit tests for read invoked without parameters and will merge into master. Thanks, everyone.

Loading

@mqudsi mqudsi merged commit 2d99623 into fish-shell:master Oct 10, 2017
1 check was pending
Loading
@zanchey zanchey added this to the fish-3.0 milestone Oct 31, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants