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

Allow cuddle assignments if used first in sub-blocks #50

Open
dionysius opened this issue Oct 23, 2019 · 4 comments
Open

Allow cuddle assignments if used first in sub-blocks #50

dionysius opened this issue Oct 23, 2019 · 4 comments

Comments

@dionysius
Copy link

dionysius commented Oct 23, 2019

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()
		}
	}
@bombsimon
Copy link
Owner

@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!

@dionysius
Copy link
Author

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.

@olibre
Copy link

olibre commented Sep 15, 2020

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.

@bombsimon bombsimon changed the title include (simple) ifs or switch for "ranges should only be cuddled with assignments used in the iteration" Allow cuddle assignments if used first in sub-blocks Mar 15, 2023
@mitar
Copy link

mitar commented Jul 24, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants