bufbuild
Use latest protocompile in `buf format`, fix things up in the formatter
There were a few bugs in the formatter that were uncovered with the new version of protocompile, mostly around trailing vs. leading comments.
For example, there was previously a bug in protocompile (https://github.com/bufbuild/protocompile/pull/33) where certain elements (like extension names enclosed in parentheses in an option name, full options declaration enclosed in brackets) would not get leading comments. The comments that should be interpreted as leading would usually instead be interpreted as a trailing comment on the previous token.
That means that there were some cases in the formatter where it was not correctly handling leading comments on certain elements (such as custom option names).
Also, this was treating leading/trailing comments for inline comments in between tokens as if the parser's classification of trailing vs. leading were authoritative. If it was a trailing comment in the AST, it would format it with no whitespace after the token, before the comment. Similarly, for leading comments, it would format it with nothing after the comment, before the token. But this classification of trailing vs. leading is just heuristic (and mostly done to mirror this distinction in source code info in descriptors generated by protoc). So now, the formatter will preserve whitespace: if a comment had no whitespace between it and a token, that will be preserved. If it did have whitespace, it will be canonicalized to a single space.
Getting everything looking right involved a greater degree of changes to the formatter than I would have liked. 🤷
The good news is that these changes should mostly be idempotent for users: they shouldn't see a lot of new diff churn from formatting with this new version. As you'll see in the new golden files, the only new differences are the whitespace around inline comments. And this change tries to preserve existing whitespace there, so if a file was previously formatted with the old version and then re-formatted with the new, there shouldn't be a diff.
The good news is that these changes should mostly be idempotent for users: they shouldn't see a lot of new diff churn from formatting with this new version. As you'll see in the new golden files, the only new differences are the whitespace around inline comments. And this change tries to preserve existing whitespace there, so if a file was previously formatted with the old version and then re-formatted with the new, there shouldn't be a diff.
Do the golden files need to be altered at all then? I only did a cursory read of the patch, but I would expect to add new tests that demonstrate preservation of whitespace, making it clear that previous formatting is still valid.
Do the golden files need to be altered at all then?
@lrewega, Yes. The old logic did not try to preserve whitespace. So the existing golden files are missing whitespace where the original source had it.
I would expect to add new tests that demonstrate preservation of whitespace,
The new golden files, compared to the existing originals, do demonstrate that.
making it clear that previous formatting is still valid.
I just did manual testing for this. I pulled all of the files into a separate directory and ran buf format -w and the golden files were not touched.
jhumphries@Joshuas-MacBook-Pro:/Users/jhumphries/src/tmp/test (master)> /Users/jhumphries/.cache/buf/Darwin/arm64/gobin/buf format -w
jhumphries@Joshuas-MacBook-Pro:/Users/jhumphries/src/tmp/test (master *)> git status | grep golden
jhumphries@Joshuas-MacBook-Pro:/Users/jhumphries/src/tmp/test (master *)> git status
On branch master
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: customoptions/options.proto
modified: proto2/enum/v1/empty.proto
modified: proto2/enum/v1/enum.proto
modified: proto2/enum/v1/enum_trailing_comment.proto
modified: proto2/enum/v1/enum_trailing_newline.proto
modified: proto2/enum/v1/enum_value_trailing_comment.proto
modified: proto2/extend/v1/empty.proto
modified: proto2/field/v1/option.proto
modified: proto2/group/v1/empty.proto
modified: proto2/header/v1/import.proto
modified: proto2/header/v1/option.proto
modified: proto2/header/v1/package.proto
modified: proto2/license/v1/enum.proto
modified: proto2/license/v1/message.proto
modified: proto2/message/v1/empty.proto
modified: proto2/message/v1/message.proto
modified: proto2/message/v1/message_extensions.proto
modified: proto2/message/v1/message_group.proto
modified: proto2/message/v1/message_import.proto
modified: proto2/message/v1/message_options.proto
modified: proto2/message/v1/multiple_nested.proto
modified: proto2/message/v1/nested.proto
modified: proto2/message/v1/sparse.proto
modified: proto2/option/v1/option.proto
modified: proto2/option/v1/option_array.proto
modified: proto2/option/v1/option_complex_array_literal.proto
modified: proto2/option/v1/option_compound_name.proto
modified: proto2/option/v1/option_message_field.proto
modified: proto2/option/v1/option_name.proto
modified: proto2/option/v1/single_compact_option.proto
modified: proto3/all/v1/all.proto
modified: proto3/file/v1/option.proto
modified: proto3/header/v1/header.proto
modified: proto3/literal/v1/compound_string.proto
modified: proto3/literal/v1/literal.proto
modified: proto3/literal/v1/literal_comments.proto
modified: proto3/literal/v1/special_literal.proto
modified: proto3/oneof/v1/multiple.proto
modified: proto3/oneof/v1/simple.proto
modified: proto3/range/v1/range.proto
modified: proto3/service/v1/empty.proto
modified: proto3/service/v1/empty_rpc.proto
modified: proto3/service/v1/service.proto
modified: proto3/service/v1/service_options.proto
modified: proto3/service/v1/simple.proto
Untracked files:
(use "git add <file>..." to include in what will be committed)
buf.yaml
no changes added to commit (use "git add" and/or "git commit -a")
Just pushed a minor commit based on feedback from @pkwarren: a "dangling" space before a semicolon looks funny. So we won't try to preserve that whitespace.
Once the CI test failure is fixed and a changelog entry is added this looks good to me. Much more readable formatting.
Okay. I think I've finally wrestled this thing into shape.
It's not pretty. I ended up making quite a lot of changes with the hope of simplifying some of the nasty edge cases and whitespace handling. I'm not sure how well I succeeded (though the last commit is a net removal of about 120 LOC...).
But I think the golden files are looking good, including with some of the additions I made to test some add'l edge cases.
@pkwarren, @bufdev, I think this is finally ready to go. Though I probably need to double-check the differences in golden outputs and amend/improve the change log wording before merging... PTAL.