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 --stdin flag to 'buf format' #1345

Closed
wants to merge 1 commit into from
Closed

Conversation

amckinney
Copy link
Contributor

Fixes #1035

This adds a --stdin flag to buf format so that users can format the .proto content read from stdin, rather than requiring a file on disk. This makes it far easier to write developer tools that would otherwise need to create a temporary file containing the buffer's content (re: #1334 (comment)).

Unsurprisingly, the --output and --write flags are not supported alongside --stdin. For anyone that needs to write the formatted content to disk, they can simply redirect the formatted result written to stdout.


stdin = bytes.NewBuffer(
[]byte(`
syntax = "proto3";
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test for multiple files concat'ed together, and either expect success or expect error? If error, document that --stdin can only take one file

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

Wait on second thought - should this not be buf format - to be consistent, since - for inputs represents stdin? Or am I getting confused? This might be a special case, but wanted to check before we merge.

@amckinney
Copy link
Contributor Author

Wait on second thought - should this not be buf format - to be consistent, since - for inputs represents stdin? Or am I getting confused? This might be a special case, but wanted to check before we merge.

Nah it's good that you called this out - I'll explain my original reasoning upfront, then we can decide if we want to change this (we probably do).

Use --stdin

I mistakenly thought the problem with leaning on - is that the input read from stdin would still need to be interpreted as a filepath - the stdin value is being read for the positional parameter (i.e. buf format <input>). For example,

buf format proto/buf/alpha/image/v1/image.proto

was thought to be equivalent to

echo proto/buf/alpha/image/v1/image.proto | buf format -

The --stdin flag was meant to tell buf that the content of the .proto file should be read from stdin, so it can adjust its operation. That's why we also return an error if the user specifies buf format some/file.proto --stdin so that it's clear the input filepath is not used at all.

Use -

Before --stdin, I actually went down the path of adding another buffetch.Format (e.g. protosource) that is only used for buf format, but ended up backing out of it since it doesn't align with the storage.Bucket abstractions tied up in buffetch and bufwire. That's solvable though.

However, now that I'm revisiting the other commands, I recall that the - input is actually interpreted as an Image / bin by default, hence:

buf build -o - | buf breaking --against -

Because buf format's default behavior for buf format - is to read the input as an Image / bin (just like the other buf commands), we'd probably want to change the meaning of the - input for buf format to be protosource format by default. The bin format isn't even supported for buf format (because it acts upon the AST), so we wouldn't actually be breaking anyone.

That way someone could just write the following instead:

cat proto/buf/alpha/image/v1/image.proto | buf format -

Which is shorthand for:

cat proto/buf/alpha/image/v1/image.proto | buf format -#format=protosource

@bufdev what are your thoughts?

Copy link
Contributor

@twilly twilly left a comment

Choose a reason for hiding this comment

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

This works but it would be nice to support process substitution and other inputs that cannot seek.

if flags.Stdin {
// We need to read the raw bytes from the io.Reader upfront so that
// the content can be referenced more than once: once by the formatter,
// and again during the diff.
Copy link
Contributor

Choose a reason for hiding this comment

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

I smell something here. --stdin is implemented with a divergent different code path because its input is a non-seekable file. Standard input is not the only non-seekable file out there: sockets, tape-archive on literal tape, named pipes, bash's named descriptors (/dev/fd/), etc.

One alternative is to implement a buffered, seeking reader. Wrap stdin with your reader and no more special code paths, I hope. I haven't groked all of the format package yet and run is particularly hard to follow.

This concept can extend further if you open a file input and attempt to seek to offset 0. That should, on POSIX systems, fail on pipes, sockets, and other file handles that are non-seekable. Commands like buf format <(cat foo.proto bar.proto) become possible.

@nedtwigg
Copy link

Sadface :( Looks like this PR isn't going to get merged, but is #1035 still planned?

@bufdev
Copy link
Member

bufdev commented Jan 19, 2023

We have no near-term plans on this as there's some thinking we need to do on it, but we may get to it in a few months. We'll keep the issue open for now, we just need the time to budget to make sure we get this right.

@bufdev bufdev deleted the amckinney/format-stdin branch February 2, 2023 19:33
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.

Format accepts input from stdin
5 participants