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

linter complains on code in generated Equal implementation #360

Closed
temoto opened this Issue Dec 2, 2017 · 12 comments

Comments

Projects
None yet
3 participants
@temoto
Contributor

temoto commented Dec 2, 2017

First, huge thanks for improved protobuf.

Code generated by gogo/slick:

func (this *Request) Equal(that interface{}) bool {
	if that == nil {
		if this == nil {
			return true
		}
		return false
	}

gosimple linter complain:

/path/bla/bla.pb.go:349:3⚠️ should use 'return ' instead of 'if { return }; return ' (gosimple)

Generated code could be reduced down to:

func (this *Request) Equal(that interface{}) bool {
	if that == nil {
		return this == nil
	}

Thanks for gogo/protobuf. Have a great time.

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Dec 3, 2017

Member

My pleasure :)

Is this the only remark the linter had?

Member

awalterschulze commented Dec 3, 2017

My pleasure :)

Is this the only remark the linter had?

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Dec 3, 2017

Contributor

Also it doesn't like repeated string literals in *valueToString() and GoString(). I guess linter wants "nil" hoisted into constant declaration.

Contributor

temoto commented Dec 3, 2017

Also it doesn't like repeated string literals in *valueToString() and GoString(). I guess linter wants "nil" hoisted into constant declaration.

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Dec 3, 2017

Member

I would be willing to review the code if you are willing to make a pull request to fix these, if possible?

Member

awalterschulze commented Dec 3, 2017

I would be willing to review the code if you are willing to make a pull request to fix these, if possible?

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Dec 3, 2017

Contributor
  • Fixing return this == nil was super easy, nice codebase.
  • Seems I'm not smart enough to fix "nil" constant. Naive approach would create duplicate declaration in sibling second.pb.go in same package.
  • Irrelevant thought on p.In()/p.Out() - wouldn't it be better to produce "raw" code without indentation and fix everything with gofmt run?
Contributor

temoto commented Dec 3, 2017

  • Fixing return this == nil was super easy, nice codebase.
  • Seems I'm not smart enough to fix "nil" constant. Naive approach would create duplicate declaration in sibling second.pb.go in same package.
  • Irrelevant thought on p.In()/p.Out() - wouldn't it be better to produce "raw" code without indentation and fix everything with gofmt run?
@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Dec 4, 2017

Member

On p.In and p.Out.
Maybe yes.
In my other code generating project goderive I generate gofmted code the first time, using p.In and p.Out. I find that running gofmt over a few 100 000 lines of code becomes quite slow and tedious.
But if you code generators output go fmted code and your editor gofmts your code when you save, then you technically don't have to ever run gofmt over your whole code base.
So in the interest of a fast compile (and code generation) feedback loop, I think we actually need to put in more of an effort in gogoprotobuf to generate gofmted code and that way we can speed up some developers' feedback loop.

Member

awalterschulze commented Dec 4, 2017

On p.In and p.Out.
Maybe yes.
In my other code generating project goderive I generate gofmted code the first time, using p.In and p.Out. I find that running gofmt over a few 100 000 lines of code becomes quite slow and tedious.
But if you code generators output go fmted code and your editor gofmts your code when you save, then you technically don't have to ever run gofmt over your whole code base.
So in the interest of a fast compile (and code generation) feedback loop, I think we actually need to put in more of an effort in gogoprotobuf to generate gofmted code and that way we can speed up some developers' feedback loop.

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Dec 4, 2017

Contributor

Understood. Have a great time.

Contributor

temoto commented Dec 4, 2017

Understood. Have a great time.

@temoto temoto closed this Dec 4, 2017

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Dec 4, 2017

Member

I hope it makes sense.

Member

awalterschulze commented Dec 4, 2017

I hope it makes sense.

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Dec 4, 2017

Contributor

#perfmatters yo 👍

Contributor

temoto commented Dec 4, 2017

#perfmatters yo 👍

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

@temoto I'm just wondering, did you explicitly ask gosimple to include generated files in its scan (because the default is false), or did it fail to detect that these were generated files?

dmitshur commented Dec 4, 2017

@temoto I'm just wondering, did you explicitly ask gosimple to include generated files in its scan (because the default is false), or did it fail to detect that these were generated files?

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Dec 4, 2017

Contributor

@shurcooL I did not run gosimple at all. It's done by gometalinter. It is quite possible that my gometalinter (with vendored linters) is too old to skip generated files. Anyway I'm quite happy that it checked and I had pleasure to get familiar with gogoprotobuf internals and remove ~4000 lines from tests fixtures.

Contributor

temoto commented Dec 4, 2017

@shurcooL I did not run gosimple at all. It's done by gometalinter. It is quite possible that my gometalinter (with vendored linters) is too old to skip generated files. Anyway I'm quite happy that it checked and I had pleasure to get familiar with gogoprotobuf internals and remove ~4000 lines from tests fixtures.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

I see, thanks.

I agree that this return this == nil change was a good simplification to make. But in the general case, I don't think that all linter advice is always appropriate for generated files; careful situational consideration should be applied.

dmitshur commented Dec 4, 2017

I see, thanks.

I agree that this return this == nil change was a good simplification to make. But in the general case, I don't think that all linter advice is always appropriate for generated files; careful situational consideration should be applied.

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Dec 5, 2017

Member

Exactly. I am open to generate more lintable code, but I will take it case by case.

Member

awalterschulze commented Dec 5, 2017

Exactly. I am open to generate more lintable code, but I will take it case by case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment