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

docs: Add proto deletion ban to style guide #6565

Closed
wants to merge 1 commit into from
Closed

Conversation

dt
Copy link
Member

@dt dt commented May 6, 2016

This change is Reviewable

@dt
Copy link
Member Author

dt commented May 6, 2016

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


STYLE.md, line 70 [r1] (raw file):
Open Question: maybe just no exceptions?


Comments from Reviewable

@RaduBerinde
Copy link
Member

Instead of the OBSOLETE_ or the bytes thing, can we recommend using reserved for the id? Seems cleaner to me if it's the same functionally. I believe it is, but we should get confirmation from others.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@bdarnell
Copy link
Contributor

bdarnell commented May 7, 2016

Cool, I didn't know the reserved option had been added. That's probably better in most cases, but if we are deleting a field that was ever written to disk, renaming is a little better: this ensures that the field can be printed correctly if we come across any old data that still contains it.

I would mention one exception to the type rule, that a message type may freely be changed to bytes. (but other than that I prefer the no-exceptions r2 to the version in r1)

LGTM

@dt
Copy link
Member Author

dt commented May 7, 2016

Done.

@RaduBerinde
Copy link
Member

LGTM

@nvanbenschoten
Copy link
Member

LGTM. I didn't know about reserved either, but I'm glad to see it's been added.

Previously, RaduBerinde wrote…

LGTM


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


STYLE.md, line 62 [r3] (raw file):

See Google's [suggested rules](https://developers.google.com/protocol-buffers/docs/proto#updating)
when modifying protos, namely:
  * Never change the type in place

s/type/type of a field/


STYLE.md, line 66 [r3] (raw file):

  * As an alternative to replacing a field with `reserved`, prefixing the name with `OBSOLETE_`
  can be used to signal deprecation (while still allowing printing in debugging, etc).
  When doing so, it is safe to change the type of an embedded message to `bytes`,

Seems like this is a contradiction of the absolute stance of rule #1. Maybe move the exception up.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Jun 16, 2016

what's the status of this?

@dt dt closed this Jun 16, 2016
@dt dt deleted the proto branch August 5, 2016 16:24
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.

6 participants