Enforce cuddling err checks

[JDiPierro]

For #65

This is the first time I've worked with an AST, it was pretty awesome... Sometimes frustrating.. but awesome :P

I set the new feature to Off by default.

[coveralls]

Pull Request Test Coverage Report for Build 163

• 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 89.319%

---

Totals
Change from base Build 155:	0.2%
Covered Lines:	577
Relevant Lines:	646

---

💛 - Coveralls

[bombsimon]

Thank you so much for this, really appreciated!

I added some comments in my review but the most important one is if we should leave the logic where this only applies to nil-checks. Like my review states I think not. If you remove that limitation it will be far less code too, e.g. no need for isCheckingErrAgainstNil. Let me know what you think!

Again, thanks! 🚀

[bombsimon]

Nice work, thanks!

I agree that AST can be a love-hate thing. But I hope my way of working with it doesn't make your first experience bad. 😃 You really have to appreciate the ast and token package from Go standard library though, very powerfull!

The next step for this package will be to work with fixers using the analysis package. I also need to find a better way to group comments and keep track of more than two statements at the same time.

[JDiPierro]

Thanks so much for the quick review!! :D I've addressed the feedback and it really did cut down on the amount of code!! I thought I'd tried some of these suggestions before and they broke other tests, but in my continual refactoring and fixing I must have corrected whatever issue resulted in that.

Since I broke the assumption that "If we made it to the case statement we're not cuddled" I had to add a few extra if cuddledWithLastStmt checks. Breaking that assumption feels kind of bad, since it complicates how you have to think about the code. My first attempt at this did the whole thing outside the case statement, but I wanted to re-use the LHS/RHS vars you already had and not have to gather those twice.

I should have time to write up some docs today :)

[JDiPierro]

Docs are done :D

Unrelated: I was thinking about pulling out all of the error strings into constants at the top of wsl.go, how would you feel about that?

[bombsimon]

Unrelated: I was thinking about pulling out all of the error strings into constants at the top of wsl.go, how would you feel about that?

I would love that, feel free to add contributions! In fact all help I can get is appreciated! Like I wrote in your issue and in this PR I feel bad for this linter to have fallen a bit behind (probably since it's a part of golangci-lint). It feels easy to implement a fixer too (regarding the complexity of the reports) but hard to do in the linters current form.

[JDiPierro]

Sorry for the force-pushes, just cleaning up some extra whitespace I noticed, didn't think they deserved their own commit :P I'm all finished with this PR if you have no further feedback :D I'll open a separate one for the error constants.

[bombsimon]

Sorry for the force-pushes, just cleaning up some extra whitespace I noticed, didn't think they deserved their own commit :P I'm all finished with this PR if you have no further feedback :D I'll open a separate one for the error constants.

No worries, force push all you want in your own branch, I don't like a messy git history either. Altough I'll probably squash when merging anyway so don't think too much about that :)

[bombsimon]

Everything seems to work as intended now!

• Tests with MustCuddleErrCheckAndAssign defualting to true
• Tests with MustCuddleErrCheckAndAssign defaulting to false
• wsl on the project with MustCuddleErrCheckAndAssign set to false - no errors
• wsl on the project with MustCuddleErrCheckAndAssign set to true - warns on line 171 (as expected)

I'll merge this but I'm not sure when I'll tag a new release. This isn't breaking since default is false but I think I want to test this out a bit since it's bundled with golangci-lint and if someone installs it with go get a patch och minor release would update the wsl dependency.

If you're using golangci-lint I hope you're OK bulding it manually to include this new feature. Eitehr way it must be configured to be set to enabled.

[JDiPierro]

Yaay, thanks for the merge, and all your help with the code!! No worries on the release tagging. I do use golangci-lint but it shouldn't be a problem to build it manually. I only use wsl in one project so far. I'm off work today but I'll try running it against our codebase tomorrow and see how it handles that :)