Skip to content

Commit

Permalink
fix(AIP-192): enforce deprecated comment on all non-file descriptors …
Browse files Browse the repository at this point in the history
…(#1233)
  • Loading branch information
noahdietz authored Aug 11, 2023
1 parent 3b9f0a2 commit efaced9
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 7 deletions.
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)
}
})
}
}

0 comments on commit efaced9

Please sign in to comment.