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

Label at the end of a block is not a whitespace (nor comment) #92

Closed
olibre opened this issue Sep 12, 2020 · 7 comments
Closed

Label at the end of a block is not a whitespace (nor comment) #92

olibre opened this issue Sep 12, 2020 · 7 comments
Labels
golangci-only Fixed in wsl but not merged to golangci-lint

Comments

@olibre
Copy link

olibre commented Sep 12, 2020

Simplified version of my code:

for _, x := range a {
	if x == y {
		goto SKIP // do not append
	}

	clean = append(clean, x)
SKIP:
}       // block should not end with a whitespace (or comment)

I think wsl considers the label SKIP as a whitespace (or comment)...

@olibre olibre changed the title label at the end of a block is not a whitespace (nor comment) Label at the end of a block is not a whitespace (nor comment) Sep 12, 2020
@bombsimon
Copy link
Owner

bombsimon commented Sep 12, 2020

I don't know why you would do this in golang though, it seems lika a pattern from BASIC och batch scripting. Maybe that says more about me but I thought I've seen my shared amount of Go code and this pattern never occurred to me. In your example I would just use continue to go to the next iteration.

for _, x := range a {
    if x == y {
        continue
    }

    clean = append(clean, x)
}

If I wanted to continue in nested loops I would label the outer most loop and continue that one like so:

OUTER:
    for range a {
        for _, x := range b {
            if x == y {
                continue OUTER
            }
        }
    }

If I had code that wasn't related to this loop that I wanted to skip i would use return. If I had code after this loop that I wanted to scope and more code after that that I wanted to execute I would break this function into several functions since that sounds like it's doing way to much.

I tried to Google and see when a good use case for goto in golang would be and I found some references to stdlib. Those however is never to go to the end of a scope and especially not to the end of a function.

So basically this is not intended but since I want this linter to enforce good practices not only by adding whitespaces but to think of what's a good way to tackle an issue and improve readability I don't see this as something that I do want to support. Ending a scope with a label just to be able to goto it is not something I want to encourage.

Obviously this is my personal opinion so feel free to argue why you think this is a good pattern and what the benefits over the other control flows such as break, countinue and return this would have.

@olibre
Copy link
Author

olibre commented Sep 13, 2020

Thank you for your excellent advise 👍
My new code:

func removeDuplicates(a, b []string) []string {
	clean := []string{}

OUTER:
	for _, x := range a {
		for _, y := range b {
			if x == y {
				goto OUTER // do not append
			}
		}

		clean = append(clean, x)
	}

	return clean
}

Note: Coding in Go is new for me 😉

@olibre
Copy link
Author

olibre commented Sep 13, 2020

Sorry for the four PR: I was thinking I was editing my own forked of your Git repo!

@Oppodelldog
Copy link
Contributor

I agree with @olibre, a jump label is not a whitespace and should not be treaten as if it was.
@bombsimon do you know what is the intention of doing so? Isn't "good coding practise" out of scope for a whitespace linter?

@bombsimon
Copy link
Owner

@Oppodelldog yep, you’re right. This should not be a part of wsl so it should be fixed, that’s why I left the issue open. This lintet should not warn about this! PRs welcome! :)

@Oppodelldog
Copy link
Contributor

@bombsimon I'll give it a try in hacktober. ;)

Oppodelldog added a commit to Oppodelldog/wsl that referenced this issue Oct 3, 2020
…t reduce by 1 line.

Now virtually collapsing empty labeled blocks will
* reveal diff in line numbers and lead to reasonBlockEndsWithWS
* allow empty labeled blocks at the end of another block (bombsimon#92)
bombsimon pushed a commit that referenced this issue Oct 3, 2020
…t reduce by 1 line.

Now virtually collapsing empty labeled blocks will
* reveal diff in line numbers and lead to reasonBlockEndsWithWS
* allow empty labeled blocks at the end of another block (#92)
@bombsimon bombsimon added the golangci-only Fixed in wsl but not merged to golangci-lint label Oct 3, 2020
@bombsimon
Copy link
Owner

Merged in golangci/golangci-lint#1750, will be available in next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
golangci-only Fixed in wsl but not merged to golangci-lint
Projects
None yet
Development

No branches or pull requests

3 participants