Producing Linter Hit List

[hollowaykeanho]

Can we produce a "hit" list for explaining each hits and how to amend them? E.g. https://go-critic.github.io/overview

I do not mind contribute back. I see a lot of good potential for this linter. =)

EDIT: added mind

[bombsimon]

Sounds great! I also think it’s possible to improve the error messages generally since they can be kind of hard to grasp.

Maybe I can start with a boilerplate and you can help me make things clearer if it’s hard to understand?

[hollowaykeanho]

Maybe I can start with a boilerplate and you can help me make things clearer if it’s hard to understand?

Perhaps the list of messages. I'll work on the infra since I already got some in hand. How would you prefer the code submission?

1. Fork --> Merge?
2. New branch --> Work on it --> Merge from branch?

[bombsimon]

Cool, thanks! Just fork this and create a PR to master whenever you feel ready!

[hollowaykeanho]

Bump.

Would you mind create an organization named wsl and add me as a contributor please? It's a requirement to create https://wsl.github.io. The subdomain must match the username/org-name.

Source: https://pages.github.com/

[hollowaykeanho]

Would you mind create an organization named wsl and add me as a contributor please? It's a requirement to create https://wsl.github.io. The subdomain must match the username/org-name.

Never mind. I managed to workaround it. The final URL should be something like: https://bombsimon.github.io/wsl/en-us

[bombsimon]

Great that you worked it out! I think it's a bit too soon/too serious to create an organisation and all that already. I can think of three ways to document this:

• In the README itself
• In a separate markdown document under a folder doc
• In the project Wiki

I don't think there's a good idea to overdo it right now and I the go-critic example you linked would be fairly easy to reproduce with regular markdown in the README to replace all the code blocks in there now.

[hollowaykeanho]

I actually managed to integrate the engine into the repository. Currently testing out at: https://hollowaykeanho.github.io/wsl/en-us/.

The write up is markdown so it's easy for you. =) Let me know your thought.

But heads up: I'm so bad at designing. Will help to scout out better design templates.

EDIT:

1. I added the documentations section for you so that it's easy to see the instructions from there. https://hollowaykeanho.github.io/wsl/en-us/contributing/documentations/
2. If you want to try it yourself, feel free to play with it. The repo is here: https://github.com/hollowaykeanho/wsl

[hollowaykeanho]

All right.. Styling completed. Please review the test site and play with it. If you're all good, I'm ready to create the merge request.

[bombsimon]

Oh, wow. Thank your for your effort! Although it's a bit much all at once going on here and I'm not sure that's exactly what I had in mind. I think a good start might be to just add the markdown to the repository just like go-critic does with their overview.

I really appreciate the work you've put in to this but it feels a bit premature and since there's not really a proper documentation without the sidebar, linking, examples etcetera maybe it's a good idea to start there. When we feel like the documentation and understanding of the linter is sufficient it might be a good idea to improve the documentation.

I also see that you introduced some kind of short name for the errors. I think this might be a good idea but before we do that it's important to define all the different error to ensure we have a logical system. If we alter the error messages we have to keep in mind that a change like that should probably result in a new major release.

I don't think it's a good idea to introduce multiple languages this early. It will add a hard dependency and I won't be able to do changes requiring the documentation to be updated. For now I think english is good enough and if someone is having a hard time understand maybe a link to Google Translate will be just as good.

I thin the concept of a logo is quite unnecessary and even though I see you have added a disclaimer I would prefer to just skip it completely, mainly since it's yet another think to maintain and handle. When the time is right I'm open to the idea of a gopher in the readme but I think that's sufficient.

So again, I really appreciate you spending time with this but I'm not ready to introduce this kind of worrk and merge it to master. The priority list now is as follows:

• Detect all false-positives and fix them
• Detect all rules that could/should/wishes to be configurable and support configuration
• Bump the version in golangci-lint asap
• Create a logic understandable markdown documentation describing the rules and how to resolve errors

[hollowaykeanho]

The priority list now is as follows:

• Detect all false-positives and fix them
• Detect all rules that could/should/wishes to be configurable and support configuration
• Bump the version in golangci-lint asap
• Create a logic understandable markdown documentation describing the rules and how to resolve > errors

If we alter the error messages we have to keep in mind that a change like that should probably result in a new major release.

Actually you have a point, I didn't set the priority correctly. Don't worry about it. The tech was pre-made there actually. It's just a few cmd instruction and you can get it back. 😄

In the README itself
In a separate markdown document under a folder doc

In that case, shall I create doc/rules.md that holds all the examples + descriptions in it? I think the README is overly cuddled and should give it some linking space. What do you think?

EDIT:
Boilerplate is ready at my fork. I removed the last fork. Please review.

Changes:

1. Link each hits in README.md after "Usage", before "Rules" - https://github.com/hollowaykeanho/wsl#checklist
2. added doc/rules.md - https://github.com/hollowaykeanho/wsl/blob/master/doc/rules.md#whitespace-linter-checklist-wsl

[bombsimon]

@hollowaykeanho Thanks, this is a great start! I think doc is fine (or docs according to standard package layout). Feel free to chose whichever (the project itself isn't currently following that layout).

Also if you want to you may remove the documentation from the README when an equivalent description is moved to the checklist.

[hollowaykeanho]

Thanks, this is a great start! I think doc is fine.

Great! I'll proceed accordingly. =)

docs according to standard package layout)

For time being I try not to touch docs because Github Pages has a strict publishing rule to it (see repo's "Settings"). I'll wait for your decision just in case you need to use that Github Pages. I will proceed with doc.

[hollowaykeanho]

Question: is the single-line block return statement correct? I'm getting a bunch of go fmt hit to produce that code.

Link: https://github.com/hollowaykeanho/wsl/blob/master/doc/rules.md#return-statements-should-not-be-cuddled-if-block-has-more-than-two-lines

Edit: Specifically looking at Sign(...) function for those who didn't run go fmt.

[bombsimon]

Not sure if I follow but the changes go fmt fixes doesn't really have anything to do with WSL. This is from the README today:

Note that this linter doesn't take in consideration the issues that will be fixed with gofmt so ensure that the code is properly formatted.

So you only have to consider code that's already formatted. The Sign example would be:

func Sign(y int) string {
    if y+2 > 0 {
        y = 0
    }

    return fmt.Sprintf("Hello world by %v\n", y)
}

The shorter version that's allowed is something like this:

func (t *Typ) SetX(x int) *Typ {
    t.X = x
    return t
}

[hollowaykeanho]

Not sure if I follow but the changes go fmt fixes doesn't really have anything to do with WSL.

The shorter version that's allowed is something like this:

Looks like I caught the wrong message. Amended.

[hollowaykeanho]

Quick question: how to get hit by branch statements should not be cuddled if block has more than two lines?

https://github.com/hollowaykeanho/wsl/blob/master/wsl.go#L341

Update: I'm currently reading through source codes to find uncommon hits.

[bombsimon]

They should all be in the README although without the name. Branch is brak or continue and since it follows the same rules as return this will result in an error:

for k, v := range aMap {
    fmt.Println(k)
    fmt.Println(v)
    break
}

Although there are probably better and more realistic usage of break and return. :)

[hollowaykeanho]

All done! Please review: https://github.com/hollowaykeanho/wsl/blob/master/doc/rules.md

Things like:

1. Grammar
2. Style

I had tested the examples on my side. Should be fine. Most of them are from my packages refactoring using the latest wsl.

If you spot something, please write it out here. I will rebase these patches again before proceeding to next phase: README touch up that seals these patches together.

[bombsimon]

@hollowaykeanho Sorry for a bit of a slow reply! I think this is a great start and I'm happy to merge it as is and do some kind of re-work from there. A few things I think at first glance:

• The examples tend to be a bit complex and makes the reader lose focus. I think they're OK now but where possible should be even simpler (with simpler types and variables)
• I saw some examples not being gofmt:ed (the var here not aligned)
• The settings available via flags are also available with settings from golangci-lint - those should be explained instead of recommending the --exclude flag.
• I really like the way you added an exception here, maybe just add a short explanation why this is (link to the issue Allow defer statements after error checking #31)
• There should be some text about slice- and index expressions (aList[index], aList[start:stop], aMap[key] etc) but I can add those later.
• The examples from the README should be removed after linking to this new document.

But all and all I think this is great and it's easy to search for the error message reported from the linter and find a way to resolve it. If you create a PR with the current state I'll happily merge it and continue the work with the minor fixes myself!

Thank you so much for your interest and help documenting this linter! 🎉

[hollowaykeanho]

I saw some examples not being gofmt:ed (the var here not aligned)

I tried it locally. This is the go fmt -s output. Was there something expected?

package main

import (
	"fmt"
)

func example(eolType int) string {
	var (
		eol = ""
		i   = 0
	)

	if eolType < 0 {
		return ""
	}

	i = eolType
	switch i {
	case 2:
		eol = "\r"
	case 3:
		eol = "\r\n"
	case 1:
		fallthrough
	default:
		eol = "\n"
	}

	return eol
}

func main() {
	fmt.Printf("%v\n", example(2))
}

I really like the way you added an exception here, maybe just add a short explanation why this is (link to the issue #31)

Applied.

The settings available via flags are also available with settings from golangci-lint - those should be explained instead of recommending the --exclude flag.

Applied.

The examples from the README should be removed after linking to this new document.

Done. That's the seal I was referring to. Pull request achieved. Glad this is done. XD

[bombsimon]

Closing this issue now that the list is in place! Enhancements will be handled outside of this issue.

Again, thank you for your help and time!