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 ability to read --{in,ex}clude value from stdin #790

Merged
merged 1 commit into from Nov 9, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented Aug 30, 2020

Today, dotnet-format accepts space-separated list of files. This
allows us to pass files via shell's native globbing expansion,
heredoc style redirections as well as via pipelines (see #551 for
examples). However, in case of pipeline it requires a second utility
(xarg) to transform pipeline input as space-separated list to
dotnet-format.

This PR implements native pipeline reading support for --include
and --exclude options, while keeping the space-separated list
intact.

Usage examples

  1. Include all C# source files under tests/Utilities directory
    that are in git source tree:

    git ls-files :/tests/Utilities/*.cs | dotnet format --include - --folder
  2. Format all C# source files in last commit:

    git show --name-only --pretty="" | dotnet format --include - --folder
  3. Exclude certain *.cs and *.vb files using ls(1):

    ls ../../../../../src/generated/{*.cs,*.vb} | dotnet format --exclude /dev/stdin --folder

Rules

  • Based on Guideline 13 of IEEE 1003.1-2017 (POSIX),
    it accepts - as an explicit marker for stdin with
    addition of:
    • /dev/stdin - which is a universal synonym of - on all Unices.
    • It only accepts explicit markers (/dev/stdin or -) and
      does not implicitly deduce the output if standard input was redirected,
      but marker was not present. This is because our usage is multi-purpose
      (both --include and --exclude).
  • It is an error if both --include and --exclude are using stdin
    marker (/dev/stdin or -).

Limitations / future considerations

  • Currently, it reads the entire input from pipeline in include/exclude
    buffer, and then runs the operation on the whole batch. In order
    to make it true pipeline friendly, it would require some refactoring;
    so files are yield return'd and enumerator can dispatch format
    operation per file.
  • At present, we do not have out-of-process functional test mechanism
    for CLI to effectively validate these kind of use-cases (redirection
    and shell globbing vs. dotnet-format's built-in globbing support);
    so no tests are included in this PR.

@am11 am11 force-pushed the feature/files-from-stdin branch 2 times, most recently from 62916ed to 4ae21eb Compare August 30, 2020 02:34
@am11 am11 changed the title Read --{in,ex}clude values from stdin Add ability to read --{in,ex}clude value from stdin Aug 30, 2020
@jmarolf
Copy link
Contributor

jmarolf commented Sep 8, 2020

@jonsequitur how difficult would it be to get support for this built into System.Commandline?

@jonsequitur
Copy link

We've been wanting to add support for stdin. This is something we've wanted to do. I don't think it would be too difficult, but we'd want to talk through the design goals a bit to come up with something generalizable and that works with the model binding infrastructure.

@jmarolf
Copy link
Contributor

jmarolf commented Sep 9, 2020

@jonsequitur got it. My preference would be to have reading from std in be a feature of system.commandline rather than us needing to special case it. I am ok with this PR going in with the assumption that we will remove this code once a more generalized solution is available.

@jonsequitur
Copy link

Agreed.

Related, this issue has been open a while: dotnet/command-line-api#275.

Today, `dotnet-format` accepts space-separated list of files. This
allows us to pass files via shell's native globbing expansion,
heredoc style redirections as well as via pipelines (see dotnet#551 for
examples). However, in case of pipeline it requires a second utility
(`xarg`) to transform pipeline input as space-separated list to
`dotnet-format`.

This PR implements native pipeline reading support for `--include`
and `--exclude` options, while keeping the space-separated list
intact.

#### Usage examples

1. Include all C# source files under `tests/Utilities` directory
that are in git source tree:

    ```sh
    git ls-files :/tests/Utilities/*.cs | dotnet format --include - --folder
    ```
3. Format all C# source files in last commit:

    ```sh
    git show --name-only --pretty="" | dotnet format --include - --folder
    ```
2. Exclude certain `*.cs` and `*.vb` files using `ls(1)`:

    ```sh
    ls ../../../../../src/generated/{*.cs,*.vb} | dotnet format --exclude /dev/stdin --folder
    ```

#### Rules

* Based on Guideline 13 of [IEEE
1003.1-2017](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02)
(POSIX),  it accepts `-` as an explicit marker for stdin with
addition of:
  * `/dev/stdin` - which is a universal synonym of `-` on all Unices.
  * It *only* accepts explicit markers (`/dev/stdin` or `-`) and
  does not implicitly deduce the output if standard input was redirected,
  but marker was not present. This is because our usage is multi-purpose
  (both `--include` and `--exclude`).
* It is an error if both `--include` and `--exclude` are using stdin
marker (`/dev/stdin` or `-`).

#### Limitations / future considerations

* Currently, it reads the entire input from pipeline in `include`/`exclude`
buffer, and then runs the operation on the whole batch. In order
to make it true pipeline friendly, it would require some refactoring;
so files are `yield return`'d and enumerator can dispatch format
operation per file.
* At present, we do not have out-of-process functional test mechanism
for CLI to effectively validate these kind of use-cases (redirection
and shell globbing vs. dotnet-format's built-in globbing support);
so no tests are included in this PR.
@am11
Copy link
Member Author

am11 commented Nov 5, 2020

@jmarolf, I will keep an eye on stdin support upstream and adapt when it happens. If these changes are fine otherwise, could this PR be merged? Thanks!

@jmarolf
Copy link
Contributor

jmarolf commented Nov 8, 2020

@am11 thanks for pinging me no this. @JoeRobich can you review this as well? If you signoff I'll merge this.

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.

None yet

4 participants