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

Breaking Change: protoc: require that reserved names are identifiers #13439

Closed

Conversation

jhump
Copy link
Contributor

@jhump jhump commented Aug 2, 2023

This changes the current warnings into errors. In the previous PR (#10586), we downgraded these to warnings with the decision that they could be upgraded back to errors in a future major version.

Now that v24.0 is on the cusp of release, this seemed like a reasonable time to change it to error.

Resolves #6335 and #4558.

@jhump jhump requested a review from a team as a code owner August 2, 2023 13:53
@jhump jhump requested review from fowles and removed request for a team August 2, 2023 13:53
@jhump
Copy link
Contributor Author

jhump commented Aug 2, 2023

@mkruskal-google, is now an appropriate time to make this change?

@fowles
Copy link
Contributor

fowles commented Aug 2, 2023

We are planning some breaking changes for the January release, so I suspect the best time for this is immediately after the October release.

@jhump
Copy link
Contributor Author

jhump commented Aug 2, 2023

We are planning some breaking changes for the January release

@fowles, would that be a good time to address grammar changes, too (#13440)?

@zhangskz zhangskz changed the title protoc: require that reserved names are identifiers Breaking Change: protoc: require that reserved names are identifiers Aug 2, 2023
@zhangskz zhangskz added protoc 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Aug 2, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Aug 2, 2023
@zhangskz
Copy link
Member

zhangskz commented Aug 2, 2023

I believe the grammar changes described in #13440 isn't a breaking change and thus wouldn't need to wait until January to be released.

@zhangskz
Copy link
Member

zhangskz commented Aug 2, 2023

Perhaps this change could be added to the PROTOBUF_FUTURE_BEAKING_CHANGES queue for the next breaking release?

#ifdef PROTOBUF_FUTURE_BREAKING_CHANGES

Copy link

github-actions bot commented Nov 7, 2023

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Nov 7, 2023
@mkruskal-google
Copy link
Member

IIRC this is the grammar change we bundled into edition 2023, so closing this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Denotes the issue/PR has not seen activity in the last 90 days. protoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

protoc: reserved names are not validated
4 participants