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

feat: Make --stdio the default for non-tty stdin #84

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

kopecs
Copy link
Contributor

@kopecs kopecs commented Mar 27, 2023

In the case where an incompatible flag is not set, --stdio will now be implied when standard input is not a terminal. This enables uses like

smlfmt < IN > OUT

without needing an additional flag.

The flag is currently left in, but I don't think it is necessary since users can simply do cat | smlfmt or similar if they'd really like to type over stdin at the terminal.

This isn't currently a breaking change since --stdio hasn't been included in a release yet, either any option (e.g., the one implemented here, or removing the explicit --stdio flag entirely) maintains backwards compatibility.

Another option would be to let - have specific meaning as an input file, designating stdin, which is common in Unix utilities, although this would presumably be a breaking change (currently this results in the usage text, so it might not be a big deal, but I'm not sure if it is cumbersome to implement with the current args parser).

I wasn't sure where changelog entries (if they do exist) should be put, so just let me know if there's a specific place/format and I can make one.

Closes #83.

In the case where an incompatible flag is not set, `--stdio` will now be
implied when standard input is not a terminal. This enables uses like
```bash
smlfmt < IN > OUT
```
without needing an additional flag.

The flag is currently left in, but I don't think it is necessary since
users can simply do `cat | smlfmt` or similar if they'd really like to
type over stdin at the terminal.

This isn't currently a breaking change since `--stdio` hasn't been
included in a release yet, either any option (e.g., the one implemented
here, or removing the explicit `--stdio` flag entirely) maintains
backwards compatibility.

Another option would be to let `-` have specific meaning as an input
file, designating stdin, which is common in Unix utilities, although
this would presumably be a breaking change (currently this results in
the usage text, so it might not be a big deal, but I'm not sure if it is
cumbersome to implement with the current args parser).
@shwestrick
Copy link
Owner

Nice!

One strange behavior is if we try to use stdin and a file input at the same time (the following output happens without any user interaction):

→ echo "val x = 5 val y = 10" | ./smlfmt test.sml
overwrite test.sml [y/N]? skipping test.sml

Can we add an error mode for asserting not stdio when a file argument is passed? I think that would fix the above problem.

Desired output would be something like this:

→ echo "val x = 5 val y = 10" | ./smlfmt test.sml
ERROR: using stdin is incompatible with passing input files 

@kopecs
Copy link
Contributor Author

kopecs commented Mar 28, 2023

Hmm. The described behaviour is what I would expect, otherwise you wouldn't be able to do e.g., yes | smlfmt <files>.

Beyond that, I think there are implementation issues with a warning here. Since we detect if stdin is to be used as input on the basis of it not being a tty, it would seem to me that users running smlfmt in a script would run into this every time they attempted to pass files as arguments. We could perhaps do something more complex where we try and test if stdin isn't a tty and also doesn't have bytes but that seems even more fragile to me.

I can make the requested changes, but does it not simply reintroduce a new problem?

@shwestrick
Copy link
Owner

shwestrick commented Mar 28, 2023

Oh, I see the problem. Hmm. This makes me think that trying to detect non-terminal input doesn't work in general. Maybe the Unix-style placeholder - is a better solution?

Are there any examples of existing tools that detect non-terminal input like we're trying to do here?

@p-ouellette
Copy link
Contributor

I think it makes sense for smlfmt to read stdin if no files are given. This is common for Unix tools (e.g. wc, cat, grep).
If you want smfmt with no files to show the usage info instead of taking input from the terminal, I don't see any issue with using isatty for this.
For example, less seems to use isatty: cat file | less reads stdin while just less at the terminal prints a usage message.
The "strange behavior" example above does not seem strange to me.

@shwestrick
Copy link
Owner

The strange behavior IMO is that the interactive confirmation is being clobbered by the piped input. smlfmt has two different modes of use for stdin: either it's for SML input, or it's for interactivity. I don't think tools like less/grep/wc have this ambiguity?

@shwestrick
Copy link
Owner

shwestrick commented Mar 29, 2023

I've thought about it some more and I think I'm coming around haha.

I was worried about getting an unintentional yes-confirmation through stdin, for overwriting a file. But this seems very unlikely... so it's probably fine.

So, moving forward, perhaps we should just get rid of --stdio? In favor of using isatty and checking whether or not there are input files.

@kopecs
Copy link
Contributor Author

kopecs commented Mar 31, 2023

I don't see an issue with removing the flag.

@shwestrick
Copy link
Owner

Sounds good. I'll go ahead and merge, and then make a note to remove the --stdio flag when we get a chance. Thanks for putting this together @kopecs!

@shwestrick shwestrick merged commit e7138e2 into shwestrick:main Mar 31, 2023
@shwestrick shwestrick mentioned this pull request Mar 31, 2023
p-ouellette added a commit to p-ouellette/smlfmt that referenced this pull request Apr 1, 2023
Standard input and output are used if no files are given and standard
input is not connected to the terminal.

Closes shwestrick#84
@p-ouellette p-ouellette mentioned this pull request Apr 1, 2023
@shwestrick shwestrick mentioned this pull request Nov 1, 2023
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.

Automatically detect if stdin isn't a terminal
3 participants