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.

Sign up for GitHub

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

Jump to bottom

Update WSL to v1.2.5 #811

Merged
merged 4 commits into from Oct 14, 2019
Merged

Update WSL to v1.2.5 #811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2096bae
Update WSL to v1.2.4
bombsimon Oct 13, 2019
8db28a6
Re-generate README.md
bombsimon Oct 13, 2019
9ce051a
Update WSL to v1.2.5
bombsimon Oct 14, 2019
831449f
Fix bad field tag for config
bombsimon Oct 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .golangci.example.yml
View file
Open in desktop
Expand Up @@ -220,6 +220,10 @@ linters-settings:
allow-assign-and-call: true
# Allow multiline assignments to be cuddled. Default is true.
allow-multiline-assign: true
# Allow case blocks to end with a whitespace.
allow-case-traling-whitespace: true
# Allow declarations (var) to be cuddled.
allow-cuddle-declarations: false

linters:
enable:
Expand Down
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -819,6 +819,10 @@ linters-settings:
allow-assign-and-call: true
# Allow multiline assignments to be cuddled. Default is true.
allow-multiline-assign: true
# Allow case blocks to end with a whitespace.
allow-case-traling-whitespace: true
# Allow declarations (var) to be cuddled.
allow-cuddle-declarations: false
linters:
enable:
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ go 1.12

require (
github.com/OpenPeeDeeP/depguard v1.0.1
github.com/bombsimon/wsl v1.2.1
github.com/bombsimon/wsl v1.2.5
github.com/fatih/color v1.7.0
github.com/go-critic/go-critic v0.3.5-0.20190904082202-d79a9f0c64db
github.com/go-lintpack/lintpack v0.5.2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Expand Up @@ -11,8 +11,8 @@ github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRF
github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q=
github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8=
github.com/bombsimon/wsl v1.2.1 h1:DcLf3V66dJi4a+KHt+F1FdOeBZ05adHqTMYFvjgv06k=
github.com/bombsimon/wsl v1.2.1/go.mod h1:43lEF/i0kpXbLCeDXL9LMT8c92HyBywXb0AsgMHYngM=
github.com/bombsimon/wsl v1.2.5 h1:9gTOkIwVtoDZywvX802SDHokeX4kW1cKnV8ZTVAPkRs=
github.com/bombsimon/wsl v1.2.5/go.mod h1:43lEF/i0kpXbLCeDXL9LMT8c92HyBywXb0AsgMHYngM=
github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk=
Expand Down
16 changes: 10 additions & 6 deletions pkg/config/config.go
View file
Open in desktop
Expand Up @@ -255,9 +255,11 @@ type GocognitSettings struct {
}

type WSLSettings struct {
StrictAppend bool `mapstructure:"strict-append"`
AllowAssignAndCallCuddle bool `mapstructure:"allow-assign-and-call"`
AllowMultiLineAssignCuddle bool `mapstructure:"allow-multiline-assign"`
StrictAppend bool `mapstructure:"strict-append"`
AllowAssignAndCallCuddle bool `mapstructure:"allow-assign-and-call"`
AllowMultiLineAssignCuddle bool `mapstructure:"allow-multiline-assign"`
AllowCaseTrailingWhitespace bool `mapstructure:"allow-case-trailing-whitespace"`
AllowCuddleDeclaration bool `mapstructure:"allow-cuddle-declarations"`
}

var defaultLintersSettings = LintersSettings{
Expand Down Expand Up @@ -289,9 +291,11 @@ var defaultLintersSettings = LintersSettings{
MinComplexity: 30,
},
WSL: WSLSettings{
StrictAppend: true,
AllowAssignAndCallCuddle: true,
AllowMultiLineAssignCuddle: true,
StrictAppend: true,
AllowAssignAndCallCuddle: true,
AllowMultiLineAssignCuddle: true,
AllowCaseTrailingWhitespace: true,
AllowCuddleDeclaration: false,
},
}

Expand Down
12 changes: 7 additions & 5 deletions pkg/golinters/wsl.go
View file
Open in desktop
Expand Up @@ -37,11 +37,13 @@ func NewWSL() *goanalysis.Linter {
files = []string{}
linterCfg = lintCtx.Cfg.LintersSettings.WSL
processorCfg = wsl.Configuration{
StrictAppend: linterCfg.StrictAppend,
AllowAssignAndCallCuddle: linterCfg.AllowAssignAndCallCuddle,
AllowMultiLineAssignCuddle: linterCfg.AllowMultiLineAssignCuddle,
AllowCuddleWithCalls: []string{"Lock", "RLock"},
AllowCuddleWithRHS: []string{"Unlock", "RUnlock"},
StrictAppend: linterCfg.StrictAppend,
AllowAssignAndCallCuddle: linterCfg.AllowAssignAndCallCuddle,
AllowMultiLineAssignCuddle: linterCfg.AllowMultiLineAssignCuddle,
AllowCaseTrailingWhitespace: linterCfg.AllowCaseTrailingWhitespace,
AllowCuddleDeclaration: linterCfg.AllowCuddleDeclaration,
AllowCuddleWithCalls: []string{"Lock", "RLock"},
AllowCuddleWithRHS: []string{"Unlock", "RUnlock"},
}
)

Expand Down
1 change: 1 addition & 0 deletions test/linters_test.go
View file
Open in desktop
Expand Up @@ -105,6 +105,7 @@ func testOneSource(t *testing.T, sourcePath string) {
"--print-issued-lines=false",
"--print-linter-name=false",
"--out-format=line-number",
"--max-same-issues=10",
}

rc := extractRunContextFromComments(t, sourcePath)
Expand Down
72 changes: 72 additions & 0 deletions test/testdata/wsl.go
View file
Open in desktop
Expand Up @@ -100,3 +100,75 @@ func f3() int {
}

func onelineShouldNotError() error { return nil }

func multilineCase() {
// Multiline cases
switch {
case true,
false:
fmt.Println("ok")
case false ||
true:
fmt.Println("ok")
case true,
false:
fmt.Println("ok")
}
}

func sliceExpr() {
// Index- and slice expressions.
var aSlice = []int{1, 2, 3}

start := 2
if v := aSlice[start]; v == 1 {
fmt.Println("ok")
}

notOk := 1
if v := aSlice[start]; v == 1 { // ERROR "if statements should only be cuddled with assignments used in the if statement itself"
fmt.Println("notOk")
fmt.Println(notOk)
}

end := 2
if len(aSlice[start:end]) > 2 {
fmt.Println("ok")
}
}

func indexExpr() {
var aMap = map[string]struct{}{"key": {}}

key := "key"
if k, ok := aMap[key]; ok {
fmt.Println(k)
}

xxx := "xxx"
if _, ok := aMap[key]; ok { // ERROR "if statements should only be cuddled with assignments used in the if statement itself"
Copy link
Member

Choose a reason for hiding this comment

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

but how to rewrite this code properly? it's the common coding pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

I see how that's a question for the linter itself and there's an issue here to allow the variable to be cuddled if it's used anywhere in the block body. Before this PR the linter gave an error even if xxx was used as the index expression. :(

The way to rewrite to satisfy the linter:

notUsedInIf := "separated from the statement"

if _, ok := aMap[key]; ok {
    fmt.Println("Not used here either")
    // Many lines later...
    fmt.Println(notUsedInIf)
}

And if the variable is used it may be cuddled.

key := "used in if"
if _, ok := aMap[key]; ok {
    fmt.Println("ok")
}

Or if it's used after the condition.

xxx := "not in if but first in block"
if _, ok := aMap[key]; ok {
    fmt.Println(xxx) // Ok
}

Copy link
Member Author

@bombsimon bombsimon Oct 14, 2019 •
edited

Choose a reason for hiding this comment

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

I got a bit confused after writing my comment and had a revisit to Effective Go. I see that a lot of the code uses a pattern that WSL discourages in favour of readability. Although I don't see newlines being discussed in the documentation and the examples doesn't state it's the only (or preferred way) to handle empty lines.

The reason for me adding it to this project is that I got asked by two of the places I've worked who uses golangci-lint and uses this kind of style to improve readability. I got a bit anxious about it since it's mainly based on personal preferences and sent and e-mail to you but I didn't get any response so I didn't revert anything.

I know it's not much but since this got merged in v1.20.0 the linter have had 10 stars and a few issues added so there seems to be a bit of an interest. Although maybe not enough to be a part of this project?

Do you think this kind of linter/styling is out of scope for golangci-lint? If so, do you wan't me to revert it being added?

If you wan't more details and thoughts behind this linter please give the README from the repository a few minutes!

Copy link
Member

Choose a reason for hiding this comment

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

got it, thank you!

fmt.Println("not ok")
fmt.Println(xxx)
}
}

func allowTrailing(i int) {
switch i {
case 1:
fmt.Println("one")

case 2:
fmt.Println("two")

case 3:
fmt.Println("three")
}
}

// ExampleSomeOutput simulates an example function.
func ExampleSomeOutput() {
fmt.Println("Hello, world")

// Output:
// Hello, world
}