-
Notifications
You must be signed in to change notification settings - Fork 48
Add a Stdin Specification #586
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||
| --- | ||||||
| outline: deep | ||||||
| --- | ||||||
|
|
||||||
| # Stdin Specification | ||||||
|
|
||||||
| Formatters **MAY** also implement the Stdin Specification, which allows | ||||||
| formatting "virtual files" passed via stdin. | ||||||
|
|
||||||
| A formatter **MUST** implement the Stdin Specification if its formatting behavior | ||||||
| can depend on the name of the file being formatted. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a bit hard to read
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but I don't know how to fix it. Does flipping the sentence around help? Suggestions appreciated.
|
||||||
|
|
||||||
| ## Rules | ||||||
|
|
||||||
| In order for the formatter to comply with this spec, it **MUST** implement the | ||||||
| vanilla [Formatter Specification](/reference/formatter-spec), and additionally | ||||||
| satisfy the following: | ||||||
|
|
||||||
| ### 1. `--stdin-filepath` flag | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
KISS?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm lightly opposed to this, for 2 reasons:
WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jfly FWIW, that sounds reasonable to me (I agree with |
||||||
|
|
||||||
| The formatter's CLI **MUST** be of the form: | ||||||
|
|
||||||
| ``` | ||||||
| <command> [options] [--stdin-filepath <path>] | ||||||
| ``` | ||||||
|
|
||||||
| Where: | ||||||
|
|
||||||
| - `<command>` is the name of the formatting tool. | ||||||
| - `[options]` are any number of flags and options that the formatter accepts. | ||||||
| - `--stdin-filepath <path>` is an optional flag that puts the formatter in | ||||||
| "stdin mode". In stdin mode, the formatter reads file contents from stdin | ||||||
| rather than the filesystem. | ||||||
| - The formatter _MAY_ alter its behavior based on the given `<path>`. For | ||||||
| example, if a there are different formatting rules in different | ||||||
| directories. If the formatter's behavior doesn't depend on the given | ||||||
| `<path>`, it's ok to ignore it. | ||||||
| - The formatter _MAY_ understand `--stdin-filepath=<path>` as well, but **MUST** | ||||||
| understand the space separated variant. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would make sense to outline the purpose of the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the path argument is not needed by the tool, it's also valid to ignore it.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've taken a stab at this. Please take a look.
Comment on lines
+31
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are tools out there whose cmdline parsers just do not support double dash flags with values. For example: https://pkg.go.dev/flag#hdr-Command_line_flag_syntax According to that documentation, any go-based formatter cannot support this without swapping away from the standard library's flags package. Treefmt itself is saved by using cobra. Here are a few examples of stdin mode from a repo of mine. I count
Given this extensive variation, I believe the structure of the command line flags should not be specified AT ALL. You should just let the user describe what the tool accepts. The way If the tool does support stdin mode, I don't see why you would ever want to run it in non-stdin mode. For example, rustfmt has both modes, but Therefore, I think you should just add a toml option saying "this tool runs in stdin mode", or perhaps just an alternate set of command line flags to use in stdin mode. Stdin mode is preferable in many cases, for example you can coordinate 5 formatter processes and pipe their stdin all the way through without buffering after every formatter. So many tools support it already with so many different flags that it's worth being flexible so this can be useful without waiting 2 years for a bunch of CLI flag adjustments to go through.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't follow this point. From the page you linked to, I see this:
What am I missing?
I hear you. We will definitely need a compatibility story, but it's TBD if that lives in treefmt itself, or treefmt-nix.
This is exactly why: invoking the formatter N / B times (where B is our batch size) rather than N times.
This is a good question. We probably kind of assume that formatters are single threaded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, it seems the docs page for go flags is just terrible. It says "the following forms are permitted" and enumerates
Please put it in treefmt. I don't want to use treefmt-nix. Sorry but I'm not learning yet another config schema, especially an opinionated one. And presuming you mean treefmt-nix would contain wrappers for all the well-known formatters to force them to fit the spec, one of the worst parts of Nix generally is that e.g. For everyday difficulty, try this example... let's say you make a typo over and over again. You have a code formatter to replace that specific typo. (This is legitimately very useful with This is instantly not compatible with the stdin spec. Basic things like this should be silently using tempfiles and not bothering me about spec compliance.
I think best solution is have a batch mode and a stdin mode. If no stdin mode is specified we use a temp file for stdin tasks. If no regular (batch mode) options are specified we use stdin when batch mode is called for. Bikeshed the options names but basically
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't see the problem here. You are correct that this tool does not implement the stdin spec, but what's the issue? It's optional to implement. From the proposed spec:
I like your There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my point is it's optional, but if you don't implement it, what does Realistically if we follow this to its conclusion, we get to either "we always pass --stdin-filepath=... and if the tool does not support it, that's their fault", or "we sometimes pass it, depending on a config flag for each formatter". Either option has a dead zone of formatters that don't support the spec when running The
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but I don't see the problem here. This is something treefmt already supports, and I'm not proposing changing that. It uses a tempfile under the hood because how else could it possibly work?
We could add a boolean for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I asked only to prompt the necessity of some kind of config, instead of unconditionally assuming it supports the spec and adding |
||||||
|
|
||||||
| Example: | ||||||
|
|
||||||
| ``` | ||||||
| $ echo "{}" | nixfmt --stdin-filepath path/to/file.nix | ||||||
| ``` | ||||||
|
|
||||||
| ### 2. Print to stdout, do not assume file is present on filesystem | ||||||
|
|
||||||
| When in stdin mode, the formatter: | ||||||
|
|
||||||
| 1. **MUST** print the formatted file to stdout. | ||||||
| 2. **MUST NOT** attempt to read the file on the filesystem. Instead, it | ||||||
| **MUST** read from stdin. | ||||||
| 3. **MUST NOT** write to the given path on the filesytem. It _MAY_ write to | ||||||
| temporary files elsewhere on disk, but _SHOULD_ clean them up when done. | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this support expressed in the treefmt.toml config?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we'd add a new
formatter.<mylanguage>.stdin(orstdin_supported) bool that defaults to false if unspecified. Feedback appreciated.Do you think the stdin spec is the place to put this? I was thinking this would go in our config docs.