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 cuddling an assignment and a variable declaration #82

Closed
grongor opened this issue May 6, 2020 · 6 comments
Closed

Allow cuddling an assignment and a variable declaration #82

grongor opened this issue May 6, 2020 · 6 comments

Comments

@grongor
Copy link

grongor commented May 6, 2020

var err error
x := 12345

I consider these two basically the same, and it's annoying that I have to split them. It makes the code look weird at some places. It's also weird when you realize that this is a perfectly fine way to do the same thing, just more verbose:

var (
    err error
    x	= 12345
)
@bombsimon
Copy link
Owner

Thanks for the report!

I don't think I agree that they are they are the same. Not in the AST and not aesthetically either. Since there are two different ways to assign (or declare) I think they should be grouped by that way and not the fact that both might result in an assignment (which is not the case in your example).

I know this is super opinionated but one of the nice benefits with the var block to me is the alignment. I guss it could be seen as an abuse but I really like this:

var (
	x          = 12345
	someLonger = 55
	andString  = "stringy"
)

Instead of this:

x := 12345
someLonger := 55
andString := "stringy"

If I want to separate declarations and assignments, that's possible in the var block like this, even though that is the kind of separation that you're against.

var (
	err error
	x   int64

	assigned   = 12345
	someLonger = 55
)

Personally I think it makes it clear and visually appealing to pick either or. Also, you have to imagine this in a bigger context. If we allow your example, this is also allowed:

var (
	err error
	x   int64
)
var foo = 10
bar := 1234
var (
	fooErr   error
	assigned = 12345
)
biz := 123
notBizOrBaz := "x"
var bizErr error

To me that's a mess and not really in line with this linters philosophy.

So to sum it up, if you want all your assignments done with := and have declarations, the way I would do it (which is how the linter allows it) would be

var err error

x := 12345

If you still want to use this linter and are using it with golangci-lint you could add an ignore regex for this specific case. One of the reason that each error is unique is to be able to identify them separately. The error is described here where you will find the string to match on.

@grongor
Copy link
Author

grongor commented May 6, 2020

I didn't want to ignore the error as I feared that I would ignore more cases than I want. So you say I can ignore the error as it is really only about this specific case?

As for your messy example...yeah, sure, looks strange. But consider this: first of all, no sane person would do that. If so, no linter can save them. Second, this is already allowed with allow-cuddle-declarations: true:

var (
	err error
	x   int64
)
var foo = 10
var (
	fooErr   error
	assigned = 12345
)
var bizErr error

How less bad that truly is? :D

I personally use var (...) syntax for large blocks of declarations/assignments, let's say 4 or more. With just two variables it seems much more verbose than it really deserves. And then this line in between those two makes no sense to me, just consider that these two are valid (when you set allow-cuddle-declarations: true):

err := error(nil)
x := 12345

var err = error(nil)
var x = 12345

So all personal feelings about prefered syntax...is my example really different, so that it should be treated separately?

var err error
x := 12345

Anyway, if you disagree, I accept that. If I can solve my issue by ignoring the error (and by that not ignoring something else), I'm fine with that :)

@grongor
Copy link
Author

grongor commented May 6, 2020

Just to clarify/simplify even more: as you wrote in the second issue, I simply consider both var and := syntaxes the same (and I think they are), so allow-cuddle-declarations shouldn't differentiate between them. I personally wouldn't allow mixing it with simple assignments =, but that's just me.

@bombsimon
Copy link
Owner

Thanks for a great response and some good comments about the situation. I think we could land somewhere in the middle but since I don't think two wrongs make a right I don't want to just abandon the current behaviour without thinking it through.

I see tow alternative ways forward here:

  1. A solution like Add option to allow cuddling of var declaration and an expression that uses that variable's reference #83
  2. Ignoring this specific linter error

Other than that I'm not sure how to address this specific issue. At least I don't think I want to do some kind of limitation on assignments and declarations if there are less than X in a row etc.

@grongor
Copy link
Author

grongor commented May 6, 2020

Sure, I'll let you decide how to move forward with this, as you know the linter better than anyone and also I can sign under the statement I don't think two wrongs make a right and we probably won't ever make perfect linter/rule. I'd go with 1) #83 :)

I'll ignore the error for now and wait for what you come up with :) Thanks for your time! Much appreciated

@bombsimon
Copy link
Owner

I'll close this in favour of #83 since that's the preferred solution with some more details about how to implement it.

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

2 participants