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

fix(AIP-192): enforce deprecated comment on all non-file descriptors #1233

Merged
merged 1 commit into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions docs/rules/0192/deprecated-comment.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,22 @@
rule:
aip: 192
name: [core, '0192', deprecated-comment]
summary: Deprecated methods must have a corresponding comment.
summary: Deprecated elements must have a corresponding comment.
permalink: /192/deprecated-comment
redirect_from:
- /0192/deprecated-comment
---

# Deprecated comments

This rule enforces that every RPC marked with the protobuf `deprecated` option
has `"Deprecated: <reason>"` as the first line in the public leading comment, as
mandated in [AIP-192][].
This rule enforces that every element marked with the protobuf `deprecated`
option has `"Deprecated: <reason>"` as the first line in the public leading
comment, as mandated in [AIP-192][].

## Details

This rule looks at each method descriptor in each proto file, and complains if
the protobuf `deprecated` option is set to `true`, but the first line of the public
This rule looks at each descriptor in each proto file, and complains if the
protobuf `deprecated` option is set to `true`, but the first line of the public
comment does not begin with "Deprecated: ".

## Examples
Expand Down
10 changes: 9 additions & 1 deletion rules/aip0192/aip0192.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,21 @@ func AddRules(r lint.RuleRegistry) error {
)
}

// Returns true if this is a deprecated method or service, false otherwise.
// Returns true if this is a deprecated descriptor, false otherwise.
func isDeprecated(d desc.Descriptor) bool {
switch d := d.(type) {
case *desc.MethodDescriptor:
return d.GetMethodOptions().GetDeprecated()
case *desc.ServiceDescriptor:
return d.GetServiceOptions().GetDeprecated()
case *desc.FieldDescriptor:
return d.GetFieldOptions().GetDeprecated()
case *desc.EnumDescriptor:
return d.GetEnumOptions().GetDeprecated()
case *desc.EnumValueDescriptor:
return d.GetEnumValueOptions().GetDeprecated()
case *desc.MessageDescriptor:
return d.GetMessageOptions().GetDeprecated()
default:
return false
}
Expand Down
112 changes: 112 additions & 0 deletions rules/aip0192/deprecated_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,115 @@ func TestDeprecatedService(t *testing.T) {
})
}
}

func TestDeprecatedField(t *testing.T) {
tests := []struct {
testName string
FieldComment string
problems testutils.Problems
}{
{"ValidFieldDeprecated", "// Deprecated: Don't use this.\n// Field comment.", nil},
{"InvalidFieldDeprecated", "// Field comment.", testutils.Problems{{Message: `Use "Deprecated: <reason>"`}}},
}

for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
message GetBookRequest {
{{.FieldComment}}
string name = 1 [deprecated = true];
}
`, test)

problems := deprecatedComment.Lint(file)
if diff := test.problems.SetDescriptor(file.GetMessageTypes()[0].GetFields()[0]).Diff(problems); diff != "" {
t.Error(diff)
}
})
}
}

func TestDeprecatedEnum(t *testing.T) {
tests := []struct {
testName string
EnumComment string
problems testutils.Problems
}{
{"ValidEnumDeprecated", "// Deprecated: Don't use this.\n// Enum comment.", nil},
{"InvalidEnumDeprecated", "// Enum comment.", testutils.Problems{{Message: `Use "Deprecated: <reason>"`}}},
}

for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
{{.EnumComment}}
enum State {
option deprecated = true;

STATE_UNSPECIFIED = 0;
}
`, test)

problems := deprecatedComment.Lint(file)
if diff := test.problems.SetDescriptor(file.GetEnumTypes()[0]).Diff(problems); diff != "" {
t.Error(diff)
}
})
}
}

func TestDeprecatedEnumValue(t *testing.T) {
tests := []struct {
testName string
EnumValueComment string
problems testutils.Problems
}{
{"ValidEnumValueDeprecated", "// Deprecated: Don't use this.\n// EnumValue comment.", nil},
{"InvalidEnumValueDeprecated", "// EnumValue comment.", testutils.Problems{{Message: `Use "Deprecated: <reason>"`}}},
}

for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
enum State {
{{.EnumValueComment}}
STATE_UNSPECIFIED = 0 [deprecated = true];
}
`, test)

problems := deprecatedComment.Lint(file)
if diff := test.problems.SetDescriptor(file.GetEnumTypes()[0].GetValues()[0]).Diff(problems); diff != "" {
t.Error(diff)
}
})
}
}

func TestDeprecatedMessage(t *testing.T) {
tests := []struct {
testName string
MessageComment string
problems testutils.Problems
}{
{"ValidMessageDeprecated", "// Deprecated: Don't use this.\n// Message comment.", nil},
{"InvalidMessageDeprecated", "// Message comment.", testutils.Problems{{Message: `Use "Deprecated: <reason>"`}}},
}

for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
{{.MessageComment}}
message GetBookRequest {
option deprecated = true;

string name = 1;
}
`, test)

problems := deprecatedComment.Lint(file)
if diff := test.problems.SetDescriptor(file.GetMessageTypes()[0]).Diff(problems); diff != "" {
t.Error(diff)
}
})
}
}