wsl

wsl copied to clipboard

Just a suggestion, I can also see it as intentional, since it would allow too complicated blocks maybe. I stumble a lot into that in my code, so I have some examples and can tackle down if and how we can improve this. I'm also very critic to myself that this suggestion is wrong and I have to overthink my code :)

what this rule wants:

	r := map[string]string{}
	for k, v := range m {
		r[k] = v
	}

I often have a range with a little if if !thisorthat()

	r := map[string]string{}
	for k, v := range m {
		if !c.IsReserved(k) {
			r[k] = v
		}
	}

Or a switch-case:

	c.Environment = make(map[string]string)
	for _, env := range req.GetConfig().GetEnvs() {
		switch env.GetKey() {
		case "user-data":
			c.CloudInitUserData = env.GetValue()
		default:
			c.Environment[env.GetKey()] = env.GetValue()
		}
	}

@dionysius Thank you for this issue! I think it's always worth discussing things like this and to get as much input as possible. After all, this linter is about opinions regarding code style and thus should be discussed!

Regarding your first example I think one idea could be to configure "maximum first block statements" for cuddled variable usages with your example requiring the depth to be 2 while the default value is 1. With 3 this would be allowed:

r := map[string]string{}
for k, v := range m {
    if true {
        if false {
            r[k] = v
        }
    }
}

Regarding the last example I think it kind of proves the case that the usage is so far away that it would be easier to not have this as a part of this rule. However, if/when I were to implement #24 my idea is to make that recursive which means that you could cuddle one variable with a block as long as it's used somewhere in said block.

I'll leave this issue open for further discussion and thoughts and maybe a first-block-depth-option would be a good combination with cuddle for entire blocks if you just want this part of it. Of course PRs are welcome!

Thank you @bombsimon for the insights! I couldn't describe it better. For me, this definitely sounds going into the right direction and am happy to hear what others think of that.

TL;DR: In my opinion, wsl should not warn when an assignment is cuddled to a for block that uses it, even if it is in a sub-sub-sub nested block.

---

Sometimes we need nested loops to fill a variable:

n := 0
for i := 0; i < 3; i++ { // for statements should only be cuddled with assignments used in the iteration
	for j := 0; j < 4; j++ {
		n += i*j
	}
}

params := []myParams{}
for _, f := range feeds { // for statements should only be cuddled with assignments used in the iteration
	for _, v := range f.vitamins {
		if v.isValid() {
			params = append(params, myParams{f.name, v.id})
		}
	}
}

Disconnecting n with its related initialization statements (n += ...) is not always a good idea. But, at the same time, adding an empty line may be a good practice, if the loop and sub-loop(s) are large. In this last case, however, the developer should refactor the loop, and use a function:

n := computeN()

params := fillWithVitamins(feeds)

In my opinion a rule based on first-block-depth-option and/or loop(s) size shifts the problem within the configuration. A good linter should not (in my opinion) require a customized configuration. So I think about how to keep wsl simple (smart but with simple and understandable rules).

So I would say the rule "for statements should only be cuddled with assignments used in the iteration" applies to the whole for block including all nested blocks. This is a simple, easy to understand rule.

And if the loop is too large, or too deep, wsl should not worry, because the correction is not to add an empty line, but to move the loop statements within a new function.

@olibre I would go even further and say that any assignments before a block of code should be allowed without a newline if it is used then somewhere (anywhere) inside the block. For for loops this is very common, but also other blocks have variables you initialize first and then manipulate inside those blocks.