Add allow-cuddle-assignment-not-used-in-block to allow cuddle assignment not used in block

[zahraKeshtkar]

No description provided.

[bombsimon]

Hi, thanks for the PR! I guess this aims to solve the comments from #24 (like #24 (comment))? Those comments refers to for loops rather than if blocks but the gist is the same.

I wonder if we should add this as an opt-out for all blocks as described in #24 (comment) and not only if statements. By only adding one case like this we would have to add multiple flags to cover more cases.

Or, alternatively, change the configuration to be a list that accepts the different kind of blocks you want to opt out, f.ex. AllowCuddleAssignment: []string{"for", "if", "range"}

If we add this and want to remove it later it would be a breaking change and require a major version bump so I need to think about what makes most sense here. Please feel free to add you thoughts if you have any!

[zahraKeshtkar]

Hi, thanks for your response. I will update my code as soon as possible.

[bombsimon]

Hi, thanks for your response. I will update my code as soon as possible.

No rush! And I've been thinking about this and I think that either you want this rule or not so I think a single opt-out (bool) to disable it for all blocks makes most sense. Using strings to represent will be tricky because we would have something like "type_switch" in that list.

So just renaming the option to something like AllowCuddledAssignmentsAndBlocks and making it apply to all blocks is the best way forward here

[zahraKeshtkar]

Hi dear Simon, I hope this message finds you well. I just wanted to let you know that I revised my code based on your suggestion. Could you please provide feedback on this pull request .
Thanks a lot.

[coveralls]

coverage: 93.73% (+0.09%) from 93.637%
when pulling 742abf3 on zahraKeshtkar:master
into 7c15727 on bombsimon:master.

[bombsimon]

Now that I read the title of this PR I realized it's a bit confusing. This change is to allow the rules of unused assignments and blocks, not to allow an arbitrary amount of assignments. Those are two different rules in wsl and I don't think we should confuse them.

I added some negative tests to ensure this is the case. I think if we want to allow both we should add a new flag, something like allow-mulltiple-assignments-and-block.

To clarify this I want to rename this flag/option to allow-cuddle-unused-assignmnet-and-block allow-cuddle-assignment-not-used-in-block to better clarify that it's only to allow the cuddling of unused assignments.

[bombsimon]

@zahraKeshtkar Thank you so much for this!

I'm ready to merge this but just want to hear your thoughts around calling it allow-cuddle-assignment-not-used-in-block. It's a bit long but hopefully you can set this in some just/make file or configure it once in the golangci-lint configuration file. WDYT?

[zahraKeshtkar]

Thank you for taking the time to provide your feedback and suggestions! I'm sorry, but I didn't quite understand your comment. Are you suggesting that we change the name of the "allow-cuddle-assignment-not-used-in-block." and ensure that this naming convention is followed throughout the entire project? Once again, thank you for your valuable input.

[bombsimon]

Sorry for the confusion. Mostly just wanted a better name for the flag since with this change we're not allowing multiple assignments even with this flag enabled and we're not allowing cuddling multiple blocks.

allow-cuddle-assignments-and-blocks (both assignments and blocks in plural) to me sounds like we support either or all of:

x := 1
y := 2
if true {}

x := 1
if true {}
if false {}

But we support none of them with this change, only these:

x := 1

y := 2
if true {}

x := 1
if true {}

if false {}

So to clarify that it's a single assignment with a single block even though it's not used in the expression I thought assignment-not-used-in-block was most descriptive, even though very long.

[zahraKeshtkar]

I completely understand your point, but I have a question. What is your opinion on supporting all of the above? I am willing to modify the code to accommodate all of them, especially this one, as it is necessary for my project.

x := 1
y := 2
if true {}

[bombsimon]

TL;DR: For version 4 we need a new flag to disable the check for multiple assignments before any block (all the Only one cuddle assignment allowed before [...]). But we should also consider a way to enable/disable any violation and if we find a good way to address that I might consider a v5.

Does this mean that merging this PR in the current state won't solve your issues and would only make sense if we add this new other flag?

---

I don't know really. Both this PR and your last suggestion is straight up against the original reason behind wsl but like the README says and like issues and comments have proven it's very strict and having it configurable can be a good thing. I like the idea of tools with very few configuration options like black but the flexibility would probably attract more people. If we find a way to make everything toggleable this tool could probably replace a bunch of other tools such as whitespace, nlreturn etc. Looking at toolls like revive there's a lot to configure and enable/disable whereas tools like gofumpt have no configurations at all.

Either way, your last suggestion is a different rule violation, it's the Only one cuddle assignment allowed before [...] rules that's on all blocks. If we would lax that and make it configurable it would have to be a different flag in the current state since I think.

I've been thinking about a v5 and what that could imply several times but never really landed in anything and therefore haven't continued on it. One thing that I do think would be good however would be to not separate all the rules but rather only have two violations

• There should be a blank line here
• There should not be a blank line here

We could then just accept a list and call each rule something, preferable something easy to understand, where you just enable them, f.ex.

allow-missing-empty-line:
  - if
  - switch
  - assign

allow-extra-empy-line:
  - block-start
  - block-end
  - multiple-assignments-before-if

In #89 we briefly touched on the subject to be able to disable any rule.

[zahraKeshtkar]

It'd be great if we could provide support for everyone, but if that's not possible, we'll proceed with this PR. I concur with you that the current name is perplexing. How about changing it to this single-assignment-not-used-in-single-block? What's your take on it?

[bombsimon]

Hey, sorry for 🐌

Yeah I also think it's nice to have it configurable, I just don't like the way the code is currently growing it its current state with all the flags. Having a way to disable everything isn't unthinkable of for me at some point.

All other flags uses the allow- prefix so I kinda want to keep it. I also think dropping the cuddle is unclear because what is it we're allowing? I don't think we need to add the single if the words are singular as in assignment and block.

I'm happy to merge this as-is if you think it's fine!