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

Update buf breaking checks to support extensions #2960

Merged
merged 7 commits into from
May 13, 2024

Conversation

jhump
Copy link
Member

@jhump jhump commented May 9, 2024

This is really two non-trivial changes. I've broken them up into multiple commits to (hopefully) make it easier to review.

  1. The first three commits are making the existing field checks (other than field deletion) support extensions.
  2. The subsequent/final two commits are adding checks for extension deletion.

The vast majority of changed lines are test data and test expectations (~1300 out of 1600 LOC). They are in their own commits (the third and fifth), in case you preferred focus a review on the other changes.

For the latter, since extensions are not defined in their containing message, they represent elements that a FILE check must verify aren't changed/deleted. A PACKAGE check allows them to move between files but not be changed/moved otherwise. These rules effectively consider the "identity" of the extension to be its fully-qualified name.

But all of the other checks assume the field's identity is its containing message plus number. That happens to be true of extensions, too: they can be uniquely identified by both fully-qualified name and by containing message + tag number.

The thing in here that might be a little odd is the FIELD_SAME_NAME check: it checks the fully-qualified name for extensions. So if you did rename an extension (or move it to a different package or nest it in a different message), you'll get a FIELD_SAME_NAME violation using just WIRE_JSON category. But you'll get that error plus an EXTENSION_NO_DELETE or PACKAGE_EXTENSION_NO_DELETE violation if using FILE or PACKAGE category. Is that weird?

@jhump jhump requested a review from bufdev May 9, 2024 18:01
@jhump jhump merged commit fb0e9ac into dev May 13, 2024
8 checks passed
@jhump jhump deleted the jh/buf-breaking-fixes-for-extension-checks branch May 13, 2024 21: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.

None yet

2 participants