Skip to content

Commit

Permalink
Merge pull request #60 from maxmind/greg/update-linting
Browse files Browse the repository at this point in the history 
Updates for new golangci-lint
  • Loading branch information
@faktas2
faktas2 authored Jun 5, 2023
2 parents 0798c25 + c54ded4 commit 1949661
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 32 deletions.
116 changes: 91 additions & 25 deletions .golangci.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"errchkjson",
"errname",
"errorlint",
# Causes a panic.
# This doesn't seem to know about CTEs or DELETEs with RETURNING
# "execinquery",
"exhaustive",
# We often don't initialize all of the struct fields. This is fine
Expand All @@ -40,18 +40,29 @@
"gomodguard",
"gosec",
"gosimple",
# This only "caught" one thing, and it seemed like a reasonable use
# of Han script. Generally, I don't think we want to prevent the use
# of particulr scripts. The time.Local checks might be useful, but
# this didn't actually catch anything of note there.
# "gosmopolitan",
"govet",
"grouper",
"ineffassign",
"lll",
# We don't use these loggers
# "loggercheck",
"makezero",
# Maintainability Index. Seems like it could be a good idea, but a
# lot of things fail and we would need to make some decisions about
# what to allow.
# "maintidx",
"misspell",
# Causes panics, e.g., when processing mmerrors
# "musttag",
"nakedret",
"nilerr",
# Perhaps too opinionated. We do have some legitimate uses of "return nil, nil"
# "nilnil",
"noctx",
"nolintlint",
# We occasionally use named returns for documentation, which is helpful.
Expand All @@ -62,13 +73,15 @@
"nosprintfhostport",
"predeclared",
"revive",
# XXX - disabled due to lack of 1.18 support.
# "rowserrcheck",
"rowserrcheck",
# https://github.com/golangci/golangci-lint/issues/287
# "safesql",
"sqlclosecheck",
"staticcheck",
"stylecheck",
# We have very few structs with multiple tags and for the couple we had, this
# actually made it harder to read.
# "tagalign",
"tenv",
"tparallel",
"typecheck",
Expand All @@ -77,12 +90,23 @@
"unused",
"usestdlibvars",
"vetshadow",
# XXX - disabled due to lack of 1.18 support.
# "wastedassign",
"wastedassign",
# We don't currently wrap external errors in this module.
# "wrapcheck",
]

# Please note that we only use depguard for stdlib as gomodguard only
# supports modules currently. See https://github.com/ryancurrah/gomodguard/issues/12
[[linters-settings.depguard.rules.main.deny]]
pkg = "io/ioutil"
desc = "Deprecated. Functions have been moved elsewhere."

[[linters-settings.depguard.rules.main.deny]]
# golang.org/x/exp/slices has better alternatives. The proposal to add this
# to Go has been accepted, https://github.com/golang/go/issues/57433
pkg = "sort"
desc = "Use golang.org/x/exp/slices instead"

[linters-settings.errcheck]
# Don't allow setting of error to the blank identifier. If there is a legitimate
# reason, there should be a nolint with an explanation.
Expand Down Expand Up @@ -117,8 +141,20 @@
[linters-settings.forbidigo]
# Forbid the following identifiers
forbid = [
"^minFraud*",
"^maxMind*",
"Geoip", # use "GeoIP"
"^geoIP", # use "geoip"
"^hubSpot", # use "hubspot"
"Maxmind", # use "MaxMind"
"^maxMind", # use "maxmind"
"Minfraud", # use "MinFraud"
"^minFraud", # use "minfraud"
"[Uu]ser[iI][dD]", # use "accountID" or "AccountID"

# use netip.ParsePrefix unless you really need a *net.IPNet
"^net.ParseCIDR",

# use netip.ParseAddr unless you really need a net.IP
"^net.ParseIP",
]

[linters-settings.gocritic]
Expand Down Expand Up @@ -199,14 +235,16 @@
# This might be good, but I don't think we want to encourage
# significant changes to regexes as we port stuff from Perl.
# "regexpSimplify",
"returnAfterHttpError",
"ruleguard",
"singleCaseSwitch",
"sliceClear",
"sloppyLen",
# This seems like it might also be good, but a lot of existing code
# fails.
# "sloppyReassign",
"returnAfterHttpError",
# This complains about helper functions in tests.
# "sloppyTestFuncName",
"sloppyTypeAssert",
"sortSlice",
"sprintfQuotedString",
Expand Down Expand Up @@ -260,6 +298,11 @@
[linters-settings.gomodguard.blocked.modules."github.com/gofrs/uuid"]
recommendations = ["github.com/google/uuid"]

[[linters-settings.gomodguard.blocked.modules]]
[linters-settings.gomodguard.blocked.modules."github.com/pelletier/go-toml"]
recommendations = ["github.com/pelletier/go-toml/v2"]
reason = "This is an outdated version."

[[linters-settings.gomodguard.blocked.modules]]
[linters-settings.gomodguard.blocked.modules."github.com/satori/go.uuid"]
recommendations = ["github.com/google/uuid"]
Expand Down Expand Up @@ -312,6 +355,10 @@
version = "< 5.0.0"
reason = "Use github.com/jackc/pgx/v5"

[[linters-settings.gomodguard.blocked.versions]]
[linters-settings.gomodguard.blocked.versions."github.com/stretchr/testify/assert"]
reason = "Use github.com/stretchr/testify/assert"

[[linters-settings.gomodguard.blocked.modules]]
[linters-settings.gomodguard.blocked.modules."inet.af/netaddr"]
recommendations = ["go4.org/netipx"]
Expand All @@ -329,9 +376,20 @@
"G307",
]


[linters-settings.govet]
# This seems to be duplicate setting, but enable it for good measure.
check-shadowing = true
"enable-all" = true

# Although it is very useful in particular cases where we are trying to
# use as little memory as possible, there are even more cases where
# other organizations may make more sense.
disable = ["fieldalignment"]

[linters-settings.govet.settings.shadow]
strict = true

[linters-settings.lll]
line-length = 120
tab-width = 4
Expand Down Expand Up @@ -373,12 +431,9 @@
# [[linters-settings.revive.rules]]
# name = "cognitive-complexity"

# This interacts poorly with gofumpt, which seems to convert
# "// " to "//<tab>". That is probably a bug in gofumpt, but there
# isn't much we can do about it right now.
# [[linters-settings.revive.rules]]
# name = "comment-spacings"
# arguments = ["easyjson", "nolint"]
[[linters-settings.revive.rules]]
name = "comment-spacings"
arguments = ["easyjson", "nolint"]

# Probably a good rule, but we have a lot of names that
# only have case differences.
Expand Down Expand Up @@ -587,23 +642,34 @@ exclude-use-default = false

[[issues.exclude-rules]]
linters = [
"govet"
"forbidigo"
]
# This refers to a minFraud field, not the MaxMind Account ID
text = "AccountUserID|Account\\.UserID"

[[issues.exclude-rules]]
linters = [
"gocritic"
]
# For some reason the imports stuff in ruleguard doesn't work in golangci-lint.
# Perhaps it has an outdated version or something
path = "_test.go"
text = "ruleguard: Prefer the alternative Context method instead"

[[issues.exclude-rules]]
linters = [
"gocritic"
]
# we want to enable almost all govet rules. It is easier to just filter out
# the ones we don't want:
#
# * fieldalignment - way too noisy. Although it is very useful in particular
# cases where we are trying to use as little memory as possible, having
# it go off on every struct isn't helpful.
# * shadow - although often useful, it complains about _many_ err
# shadowing assignments and some others where shadowing is clear.
text = "^(fieldalignment|shadow)"
# The nolintlint linter behaves oddly with ruleguard rules
source = "// *no-ruleguard"

[[issues.exclude-rules]]
linters = [
"govet"
]
text = "shadow: declaration of \"err\" shadows declaration"
# These are usually fine to shadow and not allowing shadowing for them can
# make the code unnecessarily verbose.
text = 'shadow: declaration of "(ctx|err|ok)" shadows declaration'

[[issues.exclude-rules]]
linters = [
Expand Down
15 changes: 8 additions & 7 deletions mmdbtype/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"math/big"
"math/bits"
"reflect"
//nolint:depguard // sort.Strings is not worth pulling in a new dep for
"sort"
)

Expand Down Expand Up @@ -705,16 +706,16 @@ const (
func writeCtrlByte(w writer, t DataType) (int64, error) {
size := t.size()

typeNum := t.typeNum()
typeN := t.typeNum()

var firstByte byte
var secondByte byte

if typeNum < 8 {
firstByte = byte(typeNum << 5)
if typeN < 8 {
firstByte = byte(typeN << 5)
} else {
firstByte = byte(typeNumExtended << 5)
secondByte = byte(typeNum - 7)
secondByte = byte(typeN - 7)
}

leftOver := 0
Expand Down Expand Up @@ -746,7 +747,7 @@ func writeCtrlByte(w writer, t DataType) (int64, error) {
if err != nil {
return 0, fmt.Errorf(
"writing first ctrl byte (type: %d, size: %d): %w",
typeNum,
typeN,
size,
err,
)
Expand All @@ -758,7 +759,7 @@ func writeCtrlByte(w writer, t DataType) (int64, error) {
if err != nil {
return numBytes, fmt.Errorf(
"writing second ctrl byte (type: %d, size: %d): %w",
typeNum,
typeN,
size,
err,
)
Expand All @@ -772,7 +773,7 @@ func writeCtrlByte(w writer, t DataType) (int64, error) {
if err != nil {
return numBytes, fmt.Errorf(
"writing remaining ctrl bytes (type: %d, size: %d, value: %d): %w",
typeNum,
typeN,
size,
v,
err,
Expand Down
2 changes: 2 additions & 0 deletions tree.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ func (t *Tree) insertStringNetwork(
inserterFunc inserter.Func,
node *node,
) error {
//nolint:forbidigo // code predates netip
_, ipnet, err := net.ParseCIDR(network)
if err != nil {
return fmt.Errorf("parsing network (%s): %w", network, err)
Expand All @@ -353,6 +354,7 @@ var ipv4AliasNetworks = []string{
}

func (t *Tree) insertIPv4Aliases() error {
//nolint:forbidigo // code predates netip
_, ipv4Root, err := net.ParseCIDR("::/96")
if err != nil {
return fmt.Errorf("parsing IPv4 root: %w", err)
Expand Down
11 changes: 11 additions & 0 deletions tree_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,12 +486,14 @@ func TestTreeInsertAndGet(t *testing.T) {
require.NoError(t, err)
if test.insertType == "" || test.insertType == "net" {
for _, insert := range test.inserts {
//nolint:forbidigo // code predates netip
_, network, err := net.ParseCIDR(insert.network)
require.NoError(t, err)

require.NoError(t, tree.Insert(network, insert.value))
}
for _, insert := range test.insertErrors {
//nolint:forbidigo // code predates netip
_, network, err := net.ParseCIDR(insert.network)
require.NoError(t, err)

Expand All @@ -501,16 +503,20 @@ func TestTreeInsertAndGet(t *testing.T) {
}
} else if test.insertType == "" || test.insertType == "range" {
for _, insert := range test.inserts {
//nolint:forbidigo // code predates netip
start := net.ParseIP(insert.start)
require.NotNil(t, start)
//nolint:forbidigo // code predates netip
end := net.ParseIP(insert.end)
require.NotNil(t, end)

require.NoError(t, tree.InsertRange(start, end, insert.value))
}
for _, insert := range test.insertErrors {
//nolint:forbidigo // code predates netip
start := net.ParseIP(insert.start)
require.NotNil(t, start)
//nolint:forbidigo // code predates netip
end := net.ParseIP(insert.end)
require.NotNil(t, end)

Expand All @@ -522,6 +528,7 @@ func TestTreeInsertAndGet(t *testing.T) {
tree.finalize()

for _, get := range test.gets {
//nolint:forbidigo // code predates netip
network, value := tree.Get(net.ParseIP(get.ip))

assert.Equal(t, get.expectedNetwork, network.String(), "network for %s", get.ip)
Expand Down Expand Up @@ -579,6 +586,7 @@ func checkMMDB(t *testing.T, buf *bytes.Buffer, gets []testGet, name string) {

for _, get := range gets {
var v any
//nolint:forbidigo // code predates netip
network, ok, err := reader.LookupNetwork(net.ParseIP(get.ip), &v)
require.NoError(t, err)

Expand All @@ -604,19 +612,22 @@ func TestInsertFunc_RemovalAndLaterInsert(t *testing.T) {
)
require.NoError(t, err)

//nolint:forbidigo // code predates netip
_, network, err := net.ParseCIDR("::1.1.1.0/120")
require.NoError(t, err)

value := mmdbtype.String("value")
require.NoError(t, tree.Insert(network, value))

//nolint:forbidigo // code predates netip
ip := net.ParseIP("::1.1.1.1")

recNetwork, recValue := tree.Get(ip)

assert.Equal(t, network, recNetwork)
assert.Equal(t, value, recValue)

//nolint:forbidigo // code predates netip
_, removedNetwork, err := net.ParseCIDR("::1.1.1.1/128")
require.NoError(t, err)

Expand Down

0 comments on commit 1949661

Please sign in to comment.