Skip to content

Add .txtpb support to convert command#2293

Merged
bufdev merged 8 commits intomainfrom
alfus/txtpb
Jul 17, 2023
Merged

Add .txtpb support to convert command#2293
bufdev merged 8 commits intomainfrom
alfus/txtpb

Conversation

@Alfus
Copy link
Contributor

@Alfus Alfus commented Jul 13, 2023

Note: It appears that the whitespace in the protobuf text is intentionally non-deterministic.

@Alfus Alfus requested a review from bufdev July 13, 2023 21:49
@Alfus Alfus marked this pull request as ready for review July 13, 2023 21:49
@bufdev
Copy link
Member

bufdev commented Jul 13, 2023

Ugh not this problem again...is there any way to "compact" the output smartly? We use json.Compact on protojson output as it does the same non-determinism stuff.

@Alfus
Copy link
Contributor Author

Alfus commented Jul 13, 2023

It looks like the way to disable this is in an internal package:
https://github.com/search?q=repo%3Aprotocolbuffers%2Fprotobuf-go+detrand.Disable%28%29&type=code
The test that would have depended on stability has been omitted.

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.

If we're adding the txtpb format, it needs to be added universally instead of just in bufconvert - see the previous PR that changed bin to binpb for all the places that are affected.

@Alfus
Copy link
Contributor Author

Alfus commented Jul 14, 2023

@bufdev now also integrated into the image encoding formats.

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.

This mostly LGTM, see comment

@bufdev
Copy link
Member

bufdev commented Jul 15, 2023

Looks good, let me take one last look tomorrow or Monday to make sure, unless you need merged sooner?

@Alfus
Copy link
Contributor Author

Alfus commented Jul 15, 2023

No rush, thanks for the reviews!

@bufdev
Copy link
Member

bufdev commented Jul 17, 2023

Can you bring up a corresponding PR in the buf.build repo to document this as a new format?

@bufdev
Copy link
Member

bufdev commented Jul 17, 2023

This is good to go, but needs the docs ready to go first, should be an easy change

@bufdev bufdev merged commit 4ad371a into main Jul 17, 2023
@bufdev bufdev deleted the alfus/txtpb branch July 17, 2023 19:00
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.

2 participants