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

#1002 - add "checkPublicInterface" option to "exported" rule to allow check documentation on public methods on public interfaces #1003

Merged
merged 11 commits into from
Jul 30, 2024
5 changes: 3 additions & 2 deletions RULES_DESCRIPTIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ List of all available rules.
- [call-to-gc](#call-to-gc)
- [cognitive-complexity](#cognitive-complexity)
- [comment-spacings](#comment-spacings)
- [comments-density](#comment-spacings)
- [comments-density](#comments-density)
- [confusing-naming](#confusing-naming)
- [confusing-results](#confusing-results)
- [constant-logical-expr](#constant-logical-expr)
Expand Down Expand Up @@ -474,12 +474,13 @@ Available flags are:
* _checkPrivateReceivers_ enables checking public methods of private types
* _disableStutteringCheck_ disables checking for method names that stutter with the package name (i.e. avoid failure messages of the form _type name will be used as x.XY by other packages, and that stutters; consider calling this Y_)
* _sayRepetitiveInsteadOfStutters_ replaces the use of the term _stutters_ by _repetitive_ in failure messages
* _checkPublicInterface_ enabled checking public method definitions in public interface types

Example:

```toml
[rule.exported]
arguments = ["checkPrivateReceivers", "disableStutteringCheck"]
arguments = ["checkPrivateReceivers", "disableStutteringCheck", "checkPublicInterface"]
```

## file-header
Expand Down
64 changes: 61 additions & 3 deletions rule/exported.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type ExportedRule struct {
configured bool
checkPrivateReceivers bool
disableStutteringCheck bool
checkPublicInterface bool
stuttersMsg string
sync.Mutex
}
Expand All @@ -26,7 +27,12 @@ func (r *ExportedRule) configure(arguments lint.Arguments) {
r.Lock()
if !r.configured {
var sayRepetitiveInsteadOfStutters bool
r.checkPrivateReceivers, r.disableStutteringCheck, sayRepetitiveInsteadOfStutters = r.getConf(arguments)

r.checkPrivateReceivers,
r.disableStutteringCheck,
sayRepetitiveInsteadOfStutters,
r.checkPublicInterface = r.getConf(arguments)

r.stuttersMsg = "stutters"
if sayRepetitiveInsteadOfStutters {
r.stuttersMsg = "is repetitive"
Expand Down Expand Up @@ -57,6 +63,7 @@ func (r *ExportedRule) Apply(file *lint.File, args lint.Arguments) []lint.Failur
genDeclMissingComments: make(map[*ast.GenDecl]bool),
checkPrivateReceivers: r.checkPrivateReceivers,
disableStutteringCheck: r.disableStutteringCheck,
checkPublicInterface: r.checkPublicInterface,
stuttersMsg: r.stuttersMsg,
}

Expand All @@ -70,7 +77,12 @@ func (*ExportedRule) Name() string {
return "exported"
}

func (r *ExportedRule) getConf(args lint.Arguments) (checkPrivateReceivers, disableStutteringCheck, sayRepetitiveInsteadOfStutters bool) {
func (r *ExportedRule) getConf(args lint.Arguments) (
checkPrivateReceivers,
disableStutteringCheck,
sayRepetitiveInsteadOfStutters bool,
checkPublicInterface bool,
) {
// if any, we expect a slice of strings as configuration
if len(args) < 1 {
return
Expand All @@ -88,6 +100,8 @@ func (r *ExportedRule) getConf(args lint.Arguments) (checkPrivateReceivers, disa
disableStutteringCheck = true
case "sayRepetitiveInsteadOfStutters":
sayRepetitiveInsteadOfStutters = true
case "checkPublicInterface":
checkPublicInterface = true
default:
panic(fmt.Sprintf("Unknown configuration flag %s for %s rule", flagStr, r.Name()))
}
Expand All @@ -104,6 +118,7 @@ type lintExported struct {
onFailure func(lint.Failure)
checkPrivateReceivers bool
disableStutteringCheck bool
checkPublicInterface bool
stuttersMsg string
}

Expand Down Expand Up @@ -330,11 +345,54 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor {
}
w.lintTypeDoc(v, doc)
w.checkStutter(v.Name, "type")
// Don't proceed inside types.

if w.checkPublicInterface {
if iface, ok := v.Type.(*ast.InterfaceType); ok {
if ast.IsExported(v.Name.Name) {
Comment on lines +328 to +329
Copy link
Collaborator

@denisvmedia denisvmedia Jul 29, 2024

Choose a reason for hiding this comment

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

I'd maybe change it to

Suggested change
if iface, ok := v.Type.(*ast.InterfaceType); ok {
if ast.IsExported(v.Name.Name) {
if ast.IsExported(v.Name.Name) {
if iface, ok := v.Type.(*ast.InterfaceType); ok {

This is to avoid preliminary type assertions for non-exported symbols.

P.S. And if so, two outer ifs would be possible to join as a single if (if w.checkPublicInterface && ast.IsExported(v.Name.Name)).

w.doCheckPublicInterface(v.Name.Name, iface)
}
}
}

return nil
case *ast.ValueSpec:
w.lintValueSpecDoc(v, w.lastGen, w.genDeclMissingComments)
return nil
}
return w
}

func (w *lintExported) doCheckPublicInterface(typeName string, iface *ast.InterfaceType) {
for _, m := range iface.Methods.List {
// case of ast.Ident and other implicit fields
if len(m.Names) == 0 {
continue
}
if ast.IsExported(m.Names[0].Name) {
w.lintInterfaceMethod(typeName, m)
}
}
}

func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) {
name := m.Names[0].Name
if m.Doc == nil {
w.onFailure(lint.Failure{
Node: m,
Confidence: 1,
Category: "comments",
Failure: fmt.Sprintf("public interface method %s.%s should be commented", typeName, name),
})
return
}
s := normalizeText(m.Doc.Text())
prefix := m.Names[0].Name + " "
if !strings.HasPrefix(s, prefix) {
w.onFailure(lint.Failure{
Node: m.Doc,
Confidence: 0.8,
Category: "comments",
Failure: fmt.Sprintf(`comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, prefix),
})
}
}
6 changes: 6 additions & 0 deletions test/exported_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,9 @@ func TestExportedReplacingStuttersByRepetitive(t *testing.T) {

testRule(t, "exported-issue-519", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args})
}

func TestCheckPublicInterfaceOption(t *testing.T) {
args := []any{"checkPublicInterface"}

testRule(t, "exported-issue-1002", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args})
}
24 changes: 24 additions & 0 deletions testdata/exported-issue-1002.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Package golint comment
package golint

// by default code bellow is valid,
// but if checkPublicInterface is switched on - it should check documentation in interfaces

// Some - some interface
type Some interface {
Other // should not fail
// Correct - should do all correct
Correct()
// MATCH /comment on exported interface method Some.SemiCorrect should be of the form "SemiCorrect ..."/
SemiCorrect()
NonCorrect() // MATCH /public interface method Some.NonCorrect should be commented/
}

// Other - just to check names compatibility
type Other interface {}

// for private interfaces it doesn't check docs anyway

type somePrivate interface {
AllGood()
}
Copy link
Collaborator

@denisvmedia denisvmedia Jul 29, 2024

Choose a reason for hiding this comment

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

nit (add a new line)

Suggested change
}
}

Loading