make `decorder` usable

[jabielecki]

Your feature request related to a problem? Please describe.

Currently, when a user adds the decorder linter the default configuration makes the linter silently ignore all its findings.

Consider cognitive load. For a vaguely interested user, it's already a hop to find a relevant linter in the list. Enabling that linter is another hop. Frustratingly, nothing happens. Learning about per-linter configs would be the 3rd hop and configuring this specific linter is the 4th hop.

Let's skip hops 3 and 4.

Describe the solution you'd like.

Change the above defaults for decorder in a major release (v2) to:

		DisableDecNumCheck:        true,
		DisableDecOrderCheck:      false,
		DisableInitFuncFirstCheck: false,

(Leaving the first one true as it was, but it's only a subjective suggestion.)

Describe alternatives you've considered.

Change the defaults for decorder in a minor v1.x release. This doesn't impact users who run the default set of linters, because decorder isn't one. There are however users who enable decorder and such a minor version upgrade would very likely get them new findings.

Additional context.

Example of the findings under the new settings:

type A struct{}

var my A
var yours A

type X int  # finding: type must not be placed after var

func NewA() A {
     return A{}
}

func (a A) Println() {
    fmt.Println(a)
}

func init() { # finding: init func must be the first function in file
}

[boring-cyborg]

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

[ldez]

Hello,

Currently, when a user adds the decorder linter the default configuration makes the linter silently ignore all its finding

Yes, it's the expected behavior.

#2453 (comment)

Change the defaults for decorder in a minor v1.x release. This doesn't impact users who run the default set of linters,

A lot of users use enable-all option, so there is an impact.

[jabielecki]

Yes, it's the expected behavior.

Sorry, no. As is it's unexpected behavior. I expected that linter to require 2 mental hops:

• to find a relevant linter in the list
• to enable that linter and deal with the findings (yes, I understand it is not enabled by default and very very opinionated)

It is unexpected and frustrating that in fact obtaining any findings requires two more mental hops:

• learning about per-linter configs
• configuring this specific linter

A lot of users use enable-all option, so there is an impact.

Alas, my proposition was intended towards a v2 milestone...

[ldez]

Sorry, but it's the behavior that I expect for this opinionated linter.

We can change a linter configuration without creating a v2.

golangci-lint is a meta-linter that requires choosing and configuring linters.

[jabielecki]

I'm not saying it's unexpected to the maintainer. I'm saying it's unexpected to a user.

As a user, I install a meta-linter. I take a look at a list of disabled linters. Say a line catches my eye:

decorder: check declaration order and count of types, constants, variables and functions [fast: true, auto-fix: false]

Now I enable this linter. I expect it to check declaration order. It's not out of this earth, it literally promises me that, right there. As of now, it does absolutely nothing.

[ldez]

I just want to explain a bit more why, for me, this linter is way too much opinionated and in some way wrong.
And so this explains why I choose to disable all the options by default.

---

Case 1: Methods and types

linters-settings:
  decorder:
    disable-dec-num-check: true
    disable-dec-order-check: false
    disable-init-func-first-check: true

Before

package cli

type Yo struct {
	Foo string `description:"Foo description"`
	Fii string `description:"Fii description"`
	Fuu string `description:"Fuu description"`
	Yi  *Yi    `label:"allowEmpty" file:"allowEmpty"`
	Yu  *Yi
}

func (y *Yo) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

type Yi struct {
	Foo string
	Fii string
	Fuu string
}

func (y *Yi) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

cli/fixtures_test.go:16:1: type must not be placed after func (decorder)
type Yi struct {
^

After

package cli

type Yo struct {
	Foo string `description:"Foo description"`
	Fii string `description:"Fii description"`
	Fuu string `description:"Fuu description"`
	Yi  *Yi    `label:"allowEmpty" file:"allowEmpty"`
	Yu  *Yi
}

type Yi struct {
	Foo string
	Fii string
	Fuu string
}

func (y *Yo) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

func (y *Yi) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

Splitting Struct types and methods is not a good practice: this will decrease the readability and maintainability.

---

Case 2: Grouping types

linters-settings:
  decorder:
    disable-dec-num-check: false
    disable-dec-order-check: true
    disable-init-func-first-check: true

Before

package cli

type Yo struct {
	Foo string `description:"Foo description"`
	Fii string `description:"Fii description"`
	Fuu string `description:"Fuu description"`
	Yi  *Yi    `label:"allowEmpty" file:"allowEmpty"`
	Yu  *Yi
}

func (y *Yo) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

type Yi struct {
	Foo string
	Fii string
	Fuu string
}

func (y *Yi) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

cli/fixtures_test.go:11:1: multiple "type" declarations are not allowed; use parentheses instead (decorder)
type Yi struct {
^

After

package cli

type (
	Yo struct {
		Foo string `description:"Foo description"`
		Fii string `description:"Fii description"`
		Fuu string `description:"Fuu description"`
		Yi  *Yi    `label:"allowEmpty" file:"allowEmpty"`
		Yu  *Yi
	}

	Yi struct {
		Foo string
		Fii string
		Fuu string
	}
)

func (y *Yo) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

func (y *Yi) SetDefaults() {
	y.Foo = "foo"
	y.Fii = "fii"
}

Grouping Struct types without their methods is not a good practice: this will decrease the readability and maintainability.

---

Case 3: Grouping constants

linters-settings:
  decorder:
    disable-dec-num-check: false
    disable-dec-order-check: true
    disable-init-func-first-check: true

Before

package cli

// Code of Bar.
const (
	CodeBarA = 100
	CodeBarB = 101
)

// Status of Foo.
const (
	StatusFooA = "A"
	StatusFooB = "B"
)

cli/foo.go:10:1: multiple "const" declarations are not allowed; use parentheses instead (decorder)
const (
^

After

package cli

const (
	CodeBarA = 100
	CodeBarB = 101

	StatusFooA = "A"
	StatusFooB = "B"
)

Even with an empty line, we have lost the meaning of the const groups.
Also, the "global" comments must be removed because a comment inside the group is linked to one element instead of a group.

Once again this is bad practice.

---

I will not list all the problems related to this strongly opinionated linter.

I can understand that some people want to follow this linter, but by default, we cannot suggest bad practices.

Note:

• Grouping by types is a globally bad practice. Grouping, if it's required, must be done by meaning.
• The organization of the code must be driven by readability and maintainability because the code is read by humans and not machines.

[jabielecki]

Thank you for illustrating this so well!

For the cases 2 and 3 I can only agree. This is why I began with DisableDecNumCheck: true, to specifically avoid case 2 and 3.

My intended usage was a variant of your case 1 but with added enforcement of max one type declaration per source file. After playing with it for a while, I'd say I was wrong, and the strongest reason against it is that it tends to discourage adding small private types with methods when coding a big function. ("The complexity needs to go somewhere" and in my experience it tends to go into more complex functions passing more arguments around.)