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

protoc-gen-go: indicate deprecated fields in documentation #506

Merged
merged 1 commit into from
Feb 3, 2018
Merged

protoc-gen-go: indicate deprecated fields in documentation #506

merged 1 commit into from
Feb 3, 2018

Conversation

paranoiacblack
Copy link
Contributor

Tools in the Go ecosystem treat code documented with "Deprecated:"
as special for purposes of warning users not to use them. When a
field is marked as "[deprecated=true]", generator will produce a
comment indicating the deprecated status of the field.

Fixes #396.

@paranoiacblack
Copy link
Contributor Author

Not really familiar with how these are processed by the tooling. Adding a comment to the getters might be redundant if using the underlying deprecated field in any call path triggers vet and other analyses to fail.

@paranoiacblack
Copy link
Contributor Author

@tsl0922 sorry for the noise on the original issue. Thoughts on this?

// Generate deprecation comments for getters as well.
g.P("// Deprecated: ", mname, " should not be used by new code.")
g.P("// Underlying field is marked as [deprecated=true] in ", g.file.Name)
g.P("//")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the 2 lines above needed? The end users may not care about the proto definition here.

Copy link
Contributor Author

@paranoiacblack paranoiacblack Feb 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed? I don't know. I thought it would be useful to explain why a field is deprecated, but the only reason that it is deprecated is because of the proto definition. I don't have a strong opinion either way. It seems like for the sake of documentation, the user might go look at the underlying field they were trying to get and then find the getter documentation fairly redundant. I'll probably remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dsnet
Copy link
Member

dsnet commented Feb 2, 2018

According to descriptor.proto, deprecation can be applied at multiple levels: file, message, field, enum, enum value, service, and method.

Where to put the comment is clear for most of these:

  • message: just above the type declaration.
  • field: inline with the field declaration (e.g., MyField *string // Deprecated: .... The associated getters and setters will need to be tagged as well above the func declaration.
  • enum: inline with the type declaration (e.g., MyEnum int32 // Deprecated: ....
  • enum value: inline with the const declaration (e.g., FooValue MyEnum = 5 // Deprecated: ....
  • service: just above the type declaration for the service.
  • method: just above the func declaration for the method.

The difficult one is file. Since multiple proto files can go into a single Go package, you cant simply put the "Deprecated" tag in the package comment header.

Given the verbosity. I'm okay if all it said was "Deprecated: Do not use."

@paranoiacblack
Copy link
Contributor Author

Please take another look. Newest commit handles all possible deprecated elements in a proto file as you described above. For services, this PR also emits deprecation comments on the NewGeneratedServiceClient and RegisterGeneratedServiceServer exported functions as well as the generated interface types.

@@ -0,0 +1,58 @@
// Go support for Protocol Buffers - Google's data interchange format
//
// Copyright 2010 The Go Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheeeee... Time travel. Let's party like its 2010.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=P Fixed.


option deprecated = true; // file-level deprecation

message DeprecatedRequest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment above each message, field, enum, etc to test the ordering of whether "Deprecated: " appears above, below, or inline relative to the comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, the standard library places the deprecation warning under the comment:
https://golang.org/pkg/compress/flate/#WriteError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and added test to guarantee superfluous comments are emitted when there aren't any original ones.

@@ -1330,7 +1330,11 @@ func (g *Generator) generate(file *FileDescriptor) {
// Generate the header, including package definition
func (g *Generator) generateHeader() {
g.P("// Code generated by protoc-gen-go. DO NOT EDIT.")
g.P("// source: ", g.file.Name)
if g.file.GetOptions().GetDeprecated() {
g.P("// ", g.file.Name, " is deprecated: Do not use.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

" is a deprecated file."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

deprecatedEnum := ""
if enum.GetOptions().GetDeprecated() {
// Generate inline enum deprecation comment.
deprecatedEnum = "// Deprecated: Do not use."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this an unexported constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Tools in the Go ecosystem treat code documented with "Deprecated:"
as special for purposes of warning users not to use them.  When a file,
message, field, enum, enum value, service, or method is marked as
deprecated, the generator will emit "// Deprecated: Do not use."
comments on all relevant exported Go package information related to
the deprecated element.

This is an outline of how deprecation of each element effects the generated proto:
* file - a comment is added to the top of the generated proto.
* message - a comment is added above its type definition.
* field, enum, or enum value - a comment is added inline with the defintion.
* field - an additional comment is added above the field Getter.
* service - a comment is added above generated Client interface and New method.
* service - a comment is added above generated Server interface and Registration method.
* method - a comment is added above the generated method definition.

Fixes #396.
@paranoiacblack paranoiacblack merged commit cb908bf into golang:dev Feb 3, 2018
@paranoiacblack paranoiacblack deleted the deprecated-fields branch February 3, 2018 06:59
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants