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

[Feature request] Support multiple files/dirs as command-line arguments #1696

Closed
4 of 6 tasks
pbiggar opened this issue May 5, 2021 · 11 comments · Fixed by #1724
Closed
4 of 6 tasks

[Feature request] Support multiple files/dirs as command-line arguments #1696

pbiggar opened this issue May 5, 2021 · 11 comments · Fixed by #1724

Comments

@pbiggar
Copy link
Contributor

pbiggar commented May 5, 2021

I propose we allow multiple files and directories to be listed on the command line, and for all those files/dirs to be formatted, checked, recursed, etc.

The existing way of Fantomas deals with this problem is probably the --recurse flag.

Pros and Cons

The struggle that I have here is that when I want to check that my project is formatted, the --recurse flag doesn't work for me. This is because I have many .fsx files in my .paket directory, and also I have OCaml files with the .ml/.mli suffix in the same project.

I have a script that looks through my repo and finds potential files to be formatted. Those files are put as command line arguments to the formatting tool, which works well with prettier, rustfmt, and ocamlformat.

The other way to handle this now is to call fantomas individually for each file. This works but is very very slow compared the other approach.

One alternative way to handle this would be to add a --ignore flag. This seems to be objectively worse as then we're going down the path of creating a directory traversal language, while there exist better tools for that such as find.

Examples

dotnet fantomas --check dir1 dir2 file1.fs file2.fs

This would operate the same as --recurse except it would not choose the files via recursion, just take the ones listed in the

Please provide multiple examples (before and after scenarios) and explain what rules did apply to achieve the outcome.

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

  • This is not a breaking change to Fantomas
  • I or my company would be willing to help implement and/or test this
  • This suggestion is part of the Microsoft style guide (please add a link to section if so)
  • This suggestion is part of the G-Research style guide (please add a link to section if so)
@pbiggar pbiggar changed the title [Feature request] [Feature request] Support multiple files/dirs as command-line arguments May 5, 2021
@nojaf
Copy link
Contributor

nojaf commented May 5, 2021

Hey Paul, would a .fantomasignore file help in your scenario?

@pbiggar
Copy link
Contributor Author

pbiggar commented May 5, 2021

It would help, yes.

I do think that supporting multiple files/dirs on the command line is the standard way of supporting this, and certainly it's extremely confusing that when I enter multiple files on the command line and run fantomas --check, I get told that things are fine when they are not.

@nojaf
Copy link
Contributor

nojaf commented May 6, 2021

I'm slowly getting on board with this but what would happen when you combine this with --recurse, would all folders then be processed recursively?

@pbiggar
Copy link
Contributor Author

pbiggar commented May 6, 2021

Yes, exactly. That does lead to questions like "what if I want one directory to recurse, but not another", to which the solution would be to do your own recursion with find and pass all the files you find via xargs, which is exactly what we do.

@nojaf
Copy link
Contributor

nojaf commented May 7, 2021

I'm less familiar with what find and xargs. Out of curiosity, could you post an example of how that looks.
Perhaps the --recursive flag could be deprecate in favor of solving this via the current shell.
I'm wonder how that would look like in PowerShell.

Another setting that will conflict with multiple files is the --out flag. Perhaps there should be a limitation that --out cannot be combined with multiple paths.

@pbiggar
Copy link
Contributor Author

pbiggar commented May 7, 2021

Sure, this example in inlined from https://github.com/darklang/dark/blob/main/scripts/format.

  find "$1" -type f \
    -name "*.rs" \
    -print0 \
    -o -path ".git" -prune \
    -o -path "_build" -prune \
    -o -path "./_build" -prune \
    -o -path "node_modules" -prune \
    -o -path "./node_modules" -prune \
    -o -path "containers/stroller/target" -prune \
    -o -path "./containers/stroller/target" -prune \
    -o -path "containers/queue-scheduler/target" -prune \
    -o -path "./containers/queue-scheduler/target" -prune \
    -o -path "_esy" -prune \
    -o -path "./_esy" -prune \
    -o -path "/home/dark/.esy" -prune \
    -o -path "esy.lock" -prune \
    -o -path "./esy.lock" -prune \
    -o -path "fsharp-backend/.paket" -prune \
    -o -path "./fsharp-backend/.paket" -prune \
    -o -path "fsharp-backend/Build" -prune \
    -o -path "./fsharp-backend/Build" -prune
| xargs -0 rustfmt --files-with-diff --check

@pbiggar
Copy link
Contributor Author

pbiggar commented May 7, 2021

Another setting that will conflict with multiple files is the --out flag. Perhaps there should be a limitation that --out cannot be combined with multiple paths.

That seems to be common, I would expect --out to error out (perhaps unless --out was a directory, but that seems like a level of complexity that's not helpful, at least at first)

@nojaf
Copy link
Contributor

nojaf commented May 8, 2021

Thank you for that example. I feel like the boundaries of this feature are getting clearer.

  • Extend the MainCommand to accept a list or paths. (Both files and folder).
  • The combination of --recurse would recurse all folders.
  • The combination of --out is not allowed when there are multiple paths.
  • The combination of --check should work as expected
  • The documentation would need to be updated.
  • Integration tests would need to be added.

For the next major, I would remove the --recurse setting if this feature is implemented. But that is something to follow up elsewhere.

Would you be interested in submitting a PR for this?

@pbiggar
Copy link
Contributor Author

pbiggar commented May 8, 2021

Thanks for the analysis. I probably don't have time to contribute this back right now, but might in the future.

@nojaf
Copy link
Contributor

nojaf commented May 15, 2021

Hey @pbiggar, I've published the first alpha with this feature:
https://www.nuget.org/packages/fantomas-tool/4.5.0-alpha-017
Please give it a spin to see if it works as expected.
Worked on Windows, that's all I can tell at this point.

@pbiggar
Copy link
Contributor Author

pbiggar commented Sep 14, 2021

I switched over to this method, thanks a million!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants