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

feat: implement -key-naming-case #15

Merged
merged 3 commits into from
Oct 25, 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
29 changes: 20 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ A Go linter that ensures consistent code style when using `log/slog`.

The `log/slog` API allows two different types of arguments: key-value pairs and attributes.
People may have different opinions about which one is better,
but nobody probably wants to mix them up because it makes the code harder to read.
but nobody probably wants to mix them up because it makes the code harder to read:

```go
slog.Info("a user has logged in", "user_id", 42, slog.String("ip_address", "192.0.2.0")) // ugh
Expand All @@ -26,6 +26,7 @@ The linter has several options, so you can adjust it to your own code style.
* Enforce using either key-value pairs or attributes for the entire project (optional)
* Enforce using methods that accept a context (optional)
* Enforce using constants instead of raw keys (optional)
* Enforce a single key naming convention (optional)
* Enforce putting arguments on separate lines (optional)

## 📦 Install
Expand All @@ -40,15 +41,15 @@ sloglint [flags] ./...

### Key-value pairs only

The `-kv-only` flag causes `sloglint` to report any use of attributes.
The `-kv-only` flag causes `sloglint` to report any use of attributes:

```go
slog.Info("a user has logged in", slog.Int("user_id", 42)) // sloglint: attributes should not be used
```

### Attributes only

In contrast, the `-attr-only` flag causes `sloglint` to report any use of key-value pairs.
In contrast, the `-attr-only` flag causes `sloglint` to report any use of key-value pairs:

```go
slog.Info("a user has logged in", "user_id", 42) // sloglint: key-value pairs should not be used
Expand All @@ -58,13 +59,13 @@ slog.Info("a user has logged in", "user_id", 42) // sloglint: key-value pairs sh

Some `slog.Handler` implementations make use of the given `context.Context` (e.g. to access context values).
For them to work properly, you need to pass a context to all logger calls.
The `-context-only` flag causes `sloglint` to report the use of methods without a context.
The `-context-only` flag causes `sloglint` to report the use of methods without a context:

```go
slog.Info("a user has logged in") // sloglint: methods without a context should not be used
```

This report can be fixed by using the equivalent method with the `Context` suffix.
This report can be fixed by using the equivalent method with the `Context` suffix:

```go
slog.InfoContext(ctx, "a user has logged in")
Expand All @@ -73,7 +74,8 @@ slog.InfoContext(ctx, "a user has logged in")
### No raw keys

To prevent typos, you may want to forbid the use of raw keys altogether.
The `-no-raw-keys` flag causes `sloglint` to report the use of strings as keys (including `slog.Attr` calls, e.g. `slog.Int("user_id", 42)`).
The `-no-raw-keys` flag causes `sloglint` to report the use of strings as keys
(including `slog.Attr` calls, e.g. `slog.Int("user_id", 42)`):

```go
slog.Info("a user has logged in", "user_id", 42) // sloglint: raw keys should not be used
Expand All @@ -87,7 +89,7 @@ const UserId = "user_id"
slog.Info("a user has logged in", UserId, 42)
```

...or custom `slog.Attr` constructors.
...or custom `slog.Attr` constructors:

```go
func UserId(value int) slog.Attr { return slog.Int("user_id", value) }
Expand All @@ -97,16 +99,25 @@ slog.Info("a user has logged in", UserId(42))

> 💡 Such helpers can be automatically generated for you by the [`sloggen`][2] tool. Give it a try too!

### Key naming convention

To ensure consistency in logs, you may want to enforce a single key naming convention.
The `-key-naming-case=<snake|kebab|camel|pascal>` flag causes `sloglint` to report keys written in a case other than the given one:

```go
slog.Info("a user has logged in", "user-id", 42) // sloglint: keys should be written in snake_case
```

### Arguments on separate lines

To improve code readability, you may want to put arguments on separate lines, especially when using key-value pairs.
The `-args-on-sep-lines` flag causes `sloglint` to report 2+ arguments on the same line.
The `-args-on-sep-lines` flag causes `sloglint` to report 2+ arguments on the same line:

```go
slog.Info("a user has logged in", "user_id", 42, "ip_address", "192.0.2.0") // sloglint: arguments should be put on separate lines
```

This report can be fixed by reformatting the code.
This report can be fixed by reformatting the code:

```go
slog.Info("a user has logged in",
Expand Down
5 changes: 4 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ module go-simpler.org/sloglint

go 1.20

require golang.org/x/tools v0.14.0
require (
github.com/ettle/strcase v0.1.1
golang.org/x/tools v0.14.0
)

require (
golang.org/x/mod v0.13.0 // indirect
Expand Down
12 changes: 12 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,7 +1,19 @@
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/ettle/strcase v0.1.1 h1:htFueZyVeE1XNnMEfbqp5r67qAN/4r6ya1ysq8Q+Zcw=
github.com/ettle/strcase v0.1.1/go.mod h1:hzDLsPC7/lwKyBOywSHEP89nt2pDgdy+No1NBA9o9VY=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4=
github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
golang.org/x/mod v0.13.0 h1:I/DsJXRlw/8l/0c24sM9yb0T4z9liZTduXvdAWYiysY=
golang.org/x/mod v0.13.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
golang.org/x/sync v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ=
golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE=
golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/tools v0.14.0 h1:jvNa2pY0M4r62jkRQ6RwEZZyPcymeL9XZMLBbV7U2nc=
golang.org/x/tools v0.14.0/go.mod h1:uYBEerGOWcJyEORxN+Ek8+TT266gXkNlHdJBwexUsBg=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
99 changes: 94 additions & 5 deletions sloglint.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
import (
"errors"
"flag"
"fmt"
"go/ast"
"go/token"
"go/types"
"strconv"

"github.com/ettle/strcase"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"
"golang.org/x/tools/go/ast/inspector"
Expand All @@ -17,11 +19,12 @@

// Options are options for the sloglint analyzer.
type Options struct {
KVOnly bool // Enforce using key-value pairs only (incompatible with AttrOnly).
AttrOnly bool // Enforce using attributes only (incompatible with KVOnly).
ContextOnly bool // Enforce using methods that accept a context.
NoRawKeys bool // Enforce using constants instead of raw keys.
ArgsOnSepLines bool // Enforce putting arguments on separate lines.
KVOnly bool // Enforce using key-value pairs only (incompatible with AttrOnly).
AttrOnly bool // Enforce using attributes only (incompatible with KVOnly).
ContextOnly bool // Enforce using methods that accept a context.
NoRawKeys bool // Enforce using constants instead of raw keys.
KeyNamingCase string // Enforce a single key naming convention ("snake", "kebab", "camel", or "pascal").
ArgsOnSepLines bool // Enforce putting arguments on separate lines.
}

// New creates a new sloglint analyzer.
Expand All @@ -44,6 +47,13 @@
}
}

const (
snakeCase = "snake"
kebabCase = "kebab"
tmzane marked this conversation as resolved.
Show resolved Hide resolved
camelCase = "camel"
pascalCase = "pascal"
)

func flags(opts *Options) flag.FlagSet {
fs := flag.NewFlagSet("sloglint", flag.ContinueOnError)

Expand All @@ -61,6 +71,16 @@
boolVar(&opts.NoRawKeys, "no-raw-keys", "enforce using constants instead of raw keys")
boolVar(&opts.ArgsOnSepLines, "args-on-sep-lines", "enforce putting arguments on separate lines")

fs.Func("key-naming-case", "enforce a single key naming convention (snake|kebab|camel|pascal)", func(s string) error {
switch s {
case snakeCase, kebabCase, camelCase, pascalCase:
opts.KeyNamingCase = s
return nil
default:
return fmt.Errorf("sloglint: -key-naming-case=%s: invalid value", s)

Check warning on line 80 in sloglint.go

View check run for this annotation

Codecov / codecov/patch

sloglint.go#L75-L80

Added lines #L75 - L80 were not covered by tests
}
})

return *fs
}

Expand Down Expand Up @@ -160,6 +180,17 @@
if opts.ArgsOnSepLines && argsOnSameLine(pass.Fset, call, keys, attrs) {
pass.Reportf(call.Pos(), "arguments should be put on separate lines")
}

switch {
case opts.KeyNamingCase == snakeCase && badKeyNames(pass.TypesInfo, strcase.ToSnake, keys, attrs):
pass.Reportf(call.Pos(), "keys should be written in snake_case")
case opts.KeyNamingCase == kebabCase && badKeyNames(pass.TypesInfo, strcase.ToKebab, keys, attrs):
pass.Reportf(call.Pos(), "keys should be written in kebab-case")
case opts.KeyNamingCase == camelCase && badKeyNames(pass.TypesInfo, strcase.ToCamel, keys, attrs):
pass.Reportf(call.Pos(), "keys should be written in camelCase")
case opts.KeyNamingCase == pascalCase && badKeyNames(pass.TypesInfo, strcase.ToPascal, keys, attrs):
pass.Reportf(call.Pos(), "keys should be written in PascalCase")

Check warning on line 192 in sloglint.go

View check run for this annotation

Codecov / codecov/patch

sloglint.go#L187-L192

Added lines #L187 - L192 were not covered by tests
}
})
}

Expand Down Expand Up @@ -211,6 +242,64 @@
return false
}

func badKeyNames(info *types.Info, caseFn func(string) string, keys, attrs []ast.Expr) bool {
for _, key := range keys {
if name, ok := getKeyName(key); ok && name != caseFn(name) {
return true
}
}

for _, attr := range attrs {
var expr ast.Expr
switch attr := attr.(type) {
case *ast.CallExpr: // e.g. slog.Int()
fn := typeutil.StaticCallee(info, attr)
if _, ok := attrFuncs[fn.FullName()]; ok {
expr = attr.Args[0]
}
case *ast.CompositeLit: // slog.Attr{}
switch len(attr.Elts) {
case 1: // slog.Attr{Key: ...} | slog.Attr{Value: ...}
if kv := attr.Elts[0].(*ast.KeyValueExpr); kv.Key.(*ast.Ident).Name == "Key" {
expr = kv.Value
}
case 2: // slog.Attr{..., ...} | slog.Attr{Key: ..., Value: ...}
expr = attr.Elts[0]
if kv1, ok := attr.Elts[0].(*ast.KeyValueExpr); ok && kv1.Key.(*ast.Ident).Name == "Key" {
expr = kv1.Value
}
if kv2, ok := attr.Elts[1].(*ast.KeyValueExpr); ok && kv2.Key.(*ast.Ident).Name == "Key" {
expr = kv2.Value
}
}
}
if name, ok := getKeyName(expr); ok && name != caseFn(name) {
return true
}
}

return false
}

func getKeyName(expr ast.Expr) (string, bool) {
if expr == nil {
return "", false
}
if ident, ok := expr.(*ast.Ident); ok {
if ident.Obj == nil || ident.Obj.Decl == nil || ident.Obj.Kind != ast.Con {
return "", false
}

Check warning on line 291 in sloglint.go

View check run for this annotation

Codecov / codecov/patch

sloglint.go#L290-L291

Added lines #L290 - L291 were not covered by tests
if spec, ok := ident.Obj.Decl.(*ast.ValueSpec); ok && len(spec.Values) > 0 {
// TODO: support len(spec.Values) > 1; e.g. "const foo, bar = 1, 2"
expr = spec.Values[0]
}
}
if lit, ok := expr.(*ast.BasicLit); ok && lit.Kind == token.STRING {
return lit.Value, true
}
return "", false

Check warning on line 300 in sloglint.go

View check run for this annotation

Codecov / codecov/patch

sloglint.go#L300

Added line #L300 was not covered by tests
}

func argsOnSameLine(fset *token.FileSet, call ast.Expr, keys, attrs []ast.Expr) bool {
if len(keys)+len(attrs) <= 1 {
return false // special case: slog.Info("msg", "key", "value") is ok.
Expand Down
5 changes: 5 additions & 0 deletions sloglint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ func TestAnalyzer(t *testing.T) {
analysistest.Run(t, testdata, analyzer, "no_raw_keys")
})

t.Run("key naming case", func(t *testing.T) {
analyzer := sloglint.New(&sloglint.Options{KeyNamingCase: "snake"})
analysistest.Run(t, testdata, analyzer, "key_naming_case")
})

t.Run("arguments on separate lines", func(t *testing.T) {
analyzer := sloglint.New(&sloglint.Options{ArgsOnSepLines: true})
analysistest.Run(t, testdata, analyzer, "args_on_sep_lines")
Expand Down
38 changes: 38 additions & 0 deletions testdata/src/key_naming_case/key_naming_case.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package key_naming_case
tmzane marked this conversation as resolved.
Show resolved Hide resolved

import "log/slog"

const (
snakeKey = "foo_bar"
kebabKey = "foo-bar"
)

func tests() {
slog.Info("msg")
slog.Info("msg", "foo_bar", 1)
slog.Info("msg", snakeKey, 1)
slog.Info("msg", slog.Int("foo_bar", 1))
slog.Info("msg", slog.Int(snakeKey, 1))
slog.Info("msg", slog.Attr{})
slog.Info("msg", slog.Attr{"foo_bar", slog.IntValue(1)})
slog.Info("msg", slog.Attr{snakeKey, slog.IntValue(1)})
slog.Info("msg", slog.Attr{Key: "foo_bar"})
slog.Info("msg", slog.Attr{Key: snakeKey})
slog.Info("msg", slog.Attr{Key: "foo_bar", Value: slog.IntValue(1)})
slog.Info("msg", slog.Attr{Key: snakeKey, Value: slog.IntValue(1)})
slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: "foo_bar"})
slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: snakeKey})

slog.Info("msg", "foo-bar", 1) // want `keys should be written in snake_case`
slog.Info("msg", kebabKey, 1) // want `keys should be written in snake_case`
slog.Info("msg", slog.Int("foo-bar", 1)) // want `keys should be written in snake_case`
slog.Info("msg", slog.Int(kebabKey, 1)) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{"foo-bar", slog.IntValue(1)}) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{kebabKey, slog.IntValue(1)}) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{Key: "foo-bar"}) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{Key: kebabKey}) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{Key: "foo-bar", Value: slog.IntValue(1)}) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{Key: kebabKey, Value: slog.IntValue(1)}) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: "foo-bar"}) // want `keys should be written in snake_case`
slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: kebabKey}) // want `keys should be written in snake_case`
}
8 changes: 3 additions & 5 deletions testdata/src/mixed_args/mixed_args.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,14 @@ import (
)

func tests() {
logger := slog.New(nil)
ctx := context.Background()

slog.Info("msg")
slog.Info("msg", "foo", 1)
slog.Info("msg", "foo", 1, "bar", 2)
slog.Info("msg", slog.Int("foo", 1))
slog.Info("msg", slog.Int("foo", 1), slog.Int("bar", 2))
}

func allFuncs() {
ctx := context.Background()

slog.Log(ctx, slog.LevelInfo, "msg", "foo", 1, slog.Int("bar", 2)) // want `key-value pairs and attributes should not be mixed`
slog.Debug("msg", "foo", 1, slog.Int("bar", 2)) // want `key-value pairs and attributes should not be mixed`
Expand All @@ -26,7 +25,6 @@ func allFuncs() {
slog.WarnContext(ctx, "msg", "foo", 1, slog.Int("bar", 2)) // want `key-value pairs and attributes should not be mixed`
slog.ErrorContext(ctx, "msg", "foo", 1, slog.Int("bar", 2)) // want `key-value pairs and attributes should not be mixed`

logger := slog.New(nil)
logger.Log(ctx, slog.LevelInfo, "msg", "foo", 1, slog.Int("bar", 2)) // want `key-value pairs and attributes should not be mixed`
logger.Debug("msg", "foo", 1, slog.Int("bar", 2)) // want `key-value pairs and attributes should not be mixed`
logger.Info("msg", "foo", 1, slog.Int("bar", 2)) // want `key-value pairs and attributes should not be mixed`
Expand Down
2 changes: 2 additions & 0 deletions testdata/src/no_raw_keys/no_raw_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ func tests() {
slog.Info("msg", slog.Attr{Key: foo})
slog.Info("msg", slog.Attr{Value: slog.IntValue(1)})
slog.Info("msg", slog.Attr{Key: foo, Value: slog.IntValue(1)})
slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: foo})

slog.Info("msg", "foo", 1) // want `raw keys should not be used`
slog.Info("msg", slog.Int("foo", 1)) // want `raw keys should not be used`
slog.Info("msg", slog.Attr{"foo", slog.IntValue(1)}) // want `raw keys should not be used`
slog.Info("msg", slog.Attr{Key: "foo"}) // want `raw keys should not be used`
slog.Info("msg", slog.Attr{Key: "foo", Value: slog.IntValue(1)}) // want `raw keys should not be used`
slog.Info("msg", slog.Attr{Value: slog.IntValue(1), Key: "foo"}) // want `raw keys should not be used`
}
Loading