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

Restore --stdin flag functionality #656

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

lpedrosa
Copy link
Contributor

Summary

This PR fixes the --stdin functionality which seemed to not work when the new version started using Argu for parsing the CLI arguments.

I have also preserved the original functionality where stdin input can only be read if redirected through a pipe:

# this would work
$ echo 'open System;; let () = printf "Hello World"' | fantomas-tool.exe --stdin
open System

let () = printf "Hello World"

# this would not
fantomas-tool.exe --stdin
Input path is missing...

Examples

Current master build:

$ echo 'open System;; let () = printf "Hello World"' | fantomas-tool.exe --stdin
ERROR: argument '--stdin' must be followed by <string>.
... omitted usage output

# when input is not redirected
$ fantomas-tool.exe --stdin
ERROR: argument '--stdin' must be followed by <string>.
... omitted usage output

Fantomas.CoreGlobalTool 3.1.0:

$ echo 'open System;; let () = printf "Hello World"' | dotnet fantomas --stdin
open System

let () = printf "Hello World"

# when input is not redirected
$ dotnet fantomas --stdin
Input path is missing...

This PR:

$ echo 'open System;; let () = printf "Hello World"' | fantomas-tool.exe --stdin
open System

let () = printf "Hello World"

# when input is not redirected
$ fantomas-tool.exe --stdin
Input path is missing...

Notes

The stdIn reader functionality is currently line capped (to 2000 lines, see StdLineLimit), to avoid memory overflows. The default Console.ReadLine() character limit is 256 characters, so that takes care of memory overflows.

We could potentially relax that limit, depending on how people use the --stdin functionality i.e. I am assuming this can be used as an editor hook, to format code selections
.

* Input can be read if the flag is present and it is piped
* Otherwise default to asking for input
@lpedrosa lpedrosa requested a review from nojaf January 29, 2020 13:21
@lpedrosa lpedrosa changed the title Restore --stdin flag functionality Restore --stdin flag functionality Jan 29, 2020
@lpedrosa lpedrosa mentioned this pull request Jan 29, 2020
InputPath.File input
else
InputPath.Unspecified
let maybeInput = results.TryGetResult<@ Arguments.Input @>
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, by using TryGetResult the current workaround for version can be simplified I think.
Something for another PR but thanks for this insight.

@nojaf
Copy link
Contributor

nojaf commented Jan 29, 2020

We could potentially relax that limit, depending on how people use the --stdin functionality i.e. I am assuming this can be used as an editor hook, to format code selections

For now, there aren't that many people using stdin so I would not take this into account just yet.
Editors will not use the cli to format code but the Fantomas dll are integrate it that way.

Thanks for this PR @lpedrosa !

@nojaf nojaf merged commit df45ae9 into fsprojects:master Jan 29, 2020
@lpedrosa lpedrosa deleted the fix-tool-stdin-functionality branch January 29, 2020 19:18
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.

2 participants