Fix if loop related lint failures in the deepcopy-gen #46

spzala · 2017-04-20T17:51:42Z

The changes in this patch takes care of lint failures due to unreachable
else statements. For example one of the failures is,
/zz_generated.deepcopy.go:116:10: if block ends with a return statement,
so drop this else and outdent its block

k8s-reviewable · 2017-04-20T17:51:46Z

This change is

spzala · 2017-04-20T17:56:54Z

/cc @thockin @smarterclayton - the last known changes on lint except changes we are discussing on removal of underscore in a separate PR. Also, can you please let me know what's the best way to ping you if needed on slack?, won't bug much :-). Is there a channel related to "client-gen"? Thanks!

thockin · 2017-04-20T20:02:24Z

examples/deepcopy-gen/generators/deepcopy.go

-				sw.Do("if newVal, err := c.DeepCopy(&val); err != nil {\n", nil)
-				sw.Do("return err\n", nil)
-				sw.Do("} else {\n", nil)
+				sw.Do("if newVal, err := c.DeepCopy(&val); err == nil {\n", nil)


It's much more idiomatic to test err != nil - why change that? I had a hard time realizing what you changed because my eyes just skip the pattern. I'd rather see a test for != nil and a return nil in the success path

@thockin Hi Tim, lol, sorry and I agree. Honestly I didn't like it myself too but I notice use of == nil in few other places and I was getting locally scoped error with a different solution I tried few weeks back (part of an old PR which didn't go further because of underscore related changes). But seems like a possible better solution is working now, I will update the changes. Thanks!

thockin · 2017-04-23T21:02:19Z

This is an annoying and stupid lint error. The if x := func(); x { ... } else { ... } is idiomatic and should be encouranged when x is not used outside the condition blocks.

sigh

thockin · 2017-04-23T21:02:38Z

change LGTM but fails travis

thockin · 2017-04-23T21:03:02Z

restarted travis - 1/3 failed

spzala · 2017-04-23T22:56:27Z

@thockin I know :( and thanks much!! It seems like no straightforward error with Travis? So I guess may be one more restart on a weekday?

thockin · 2017-04-23T23:22:52Z

It is failing against Go tip, consistently. I am on mobile, so can't investigate.

--- FAIL: TestSnippetWriter (0.00s)
	snippet_writer_test.go:85: Expected "snippet_writer_test.go:78" but didn't find it in "template: /home/travis/gopath/src/k8s.io/gengo/generator/snippet_writer_test.go:77:1: unclosed action"
FAIL
FAIL	k8s.io/gengo/generator	0.006s

spzala · 2017-04-23T23:47:59Z

@thockin Thanks Tim and no rush!! I noticed that it was the same failure even in previous patch with no code change (i.e. only a comment was added #45 ). Seems strange :(

thockin · 2017-04-24T18:50:16Z

It looks like something change in upstream Go wrt line numbers.

Can you fold in a change (separate commit) to generator/snippet_writer_test.go to remove the literal line number? and see if that passes?

spzala · 2017-04-24T20:55:33Z

@thockin Hi Tim, very nice, that seems did the trick .. I created this one #48 and Travis's happy. Thanks!!

thockin · 2017-04-25T00:52:26Z

rebase and ping me

spzala · 2017-04-27T13:47:07Z

@thockin Hi Tim, I tried pinging you on slack kube-dev couple times but without much luck of talking to you :-) Just an update that this PR is passing Travis. Thanks!!

thockin · 2017-04-27T16:29:19Z

I just restarted travis - I added tests that this will have to update. Resync to master and look at examples/deepcopy/output_tests

…

On Thu, Apr 27, 2017 at 6:47 AM, Sahdev Zala ***@***.***> wrote: @thockin <https://github.com/thockin> Hi Tim, I tried pinging you on slack kube-dev couple times but without much luck of talking to you :-) Just an update that this PR is passing Travis. Thanks!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVCJ8as6bkS-rYJcf1o2lLp6p6cDiks5r0JxbgaJpZM4NDXWH> .

thockin · 2017-04-27T16:29:40Z

Specifically, run `make test` in that dir.

…

On Thu, Apr 27, 2017 at 9:28 AM, Tim Hockin ***@***.***> wrote: I just restarted travis - I added tests that this will have to update. Resync to master and look at examples/deepcopy/output_tests On Thu, Apr 27, 2017 at 6:47 AM, Sahdev Zala ***@***.***> wrote: > @thockin <https://github.com/thockin> Hi Tim, I tried pinging you on > slack kube-dev couple times but without much luck of talking to you :-) > Just an update that this PR is passing Travis. Thanks!! > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#46 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AFVgVCJ8as6bkS-rYJcf1o2lLp6p6cDiks5r0JxbgaJpZM4NDXWH> > . >

spzala · 2017-04-27T18:04:42Z

Thanks Tim @thockin

The changes in this patch takes care of lint failures due to unreachable else statements. For example one of the failures is, /zz_generated.deepcopy.go:116:10: if block ends with a return statement, so drop this else and outdent its block

spzala · 2017-04-27T21:05:19Z

@thockin is there a way I can restart Travis job by adding a comment or something like that? (I did rebase and updated PR but seems like latest Travis failure is due to incomplete build job and probably need a restart) Thanks!

thockin · 2017-04-27T21:08:01Z

like that

spzala · 2017-04-27T21:16:00Z

Ahhh.. interesting.. Close the PR and ReOpen. Different but that's cool :-) Thanks @thockin !!

spzala · 2017-04-27T22:24:51Z

@thockin Yay :-) it passed now.

thockin · 2017-04-27T23:35:10Z

Curiously, no test cases exercize this. I'm going to merge it because that's not your fault, but if you want to make me extra happy, maybe you could throw together super-minimal output_test cases that expose this change?

spzala · 2017-04-27T23:40:56Z

@thockin :-) Thanks Tim! And yes sure, I would work on it. I am traveling on business for a week and preparing for my speaking session etc but I believe there is no super rush with it right? I will be working on soon I can. Thanks much!!

thockin · 2017-04-27T23:50:32Z

Also it should be SUPER EASY. Copy on of the directories, edit doc.g to have a type that highlights this case, run `make test`

…

On Thu, Apr 27, 2017 at 4:40 PM, Sahdev Zala ***@***.***> wrote: @thockin <https://github.com/thockin> :-) Thanks Tim! And yes sure, I would work on it. I am traveling on business for a week and preparing for my speaking session etc but I believe there is no super rush with it right? I will be working on soon I can. Thanks much!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVA_sAj0-UIqyv6oGv1u9niwdc6g2ks5r0SeJgaJpZM4NDXWH> .

spzala · 2017-04-28T00:21:51Z

Awesome, thanks @thockin !!

Add new test convering servicecatalog types related to changes made under kubernetes#46

Add new tests convering types related to changes made under kubernetes#46

liggitt · 2017-05-15T01:13:32Z

examples/deepcopy-gen/generators/deepcopy.go

@@ -523,11 +523,12 @@ func (g *genDeepCopy) doMap(t *types.Type, sw *generator.SnippetWriter) {
 				sw.Do("}\n", nil)
 				sw.Do("(*out)[key] = *newVal\n", nil)
 			} else {
-				sw.Do("if newVal, err := c.DeepCopy(&val); err != nil {\n", nil)
+				sw.Do("newVal, err := c.DeepCopy(&val)\n", nil)


this PR produces code that will not compile

see https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/45797/pull-kubernetes-bazel/20933/

I0515 01:05:35.400] k8s.io/kubernetes/vendor/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/zz_generated.deepcopy.go:59: no new variables on left side of :=

func DeepCopy_apiextensions_CustomResource(in interface{}, out interface{}, c *conversion.Cloner) error { { in := in.(*CustomResource) out := out.(*CustomResource) *out = *in newVal, err := c.DeepCopy(&in.ObjectMeta) if err != nil { return err } out.ObjectMeta = *newVal.(*v1.ObjectMeta) newVal, err := c.DeepCopy(&in.Spec) if err != nil { return err } out.Spec = *newVal.(*CustomResourceSpec) newVal, err := c.DeepCopy(&in.Status) if err != nil { return err } out.Status = *newVal.(*CustomResourceStatus) return nil } }

opened #56 to revert so master is usable (we need to bump as a prereq of kubernetes/kubernetes#45294)... can revisit this if we decide we want to jump through scoping or variable name hoops to avoid this lint error (I personally don't think it's worth it)

@liggitt thanks for fixing it Jordan. My bad, in my manual verification of generated code and build I apparently didn't run into it. I guess Gengo Travis passed because the method with nil return is not in Gengo but K8s. @thockin was quick in seeing a need for tests when lint related these changes were merged and kindly created new tests but those tests still missed few scenarios. I have also create a follow up PR (#51) to add new tests. I value your opinion on spending time for lint may not worth :-) but if we can fix existing code on lint and going forward have lint complied code then that will reduce skipping lint check and produce more clean code. Thanks!!

lint is a moving target (new lint criteria get added over time), and (as in this case) sometimes encourages non-idiomatic code. I don't think jumping through hoops to silence it is a good idea.

I have also create a follow up PR (#51) to add new tests.

I don't think that test exercises this case, either... this error was caused by multiple fields in the deep-copied type resulting in newVal being redeclared multiple times in the same scope.

@liggitt about lint agree as long as we try to catch it manually but at the same time I guess what can be worth is to automate test process against lint (latest) as part of the build (something similar to what's done with PEP 8 in projects like OpenStack), that should be a real solution.

@liggitt for #51 do you think whatever is added in the PR are valuable tests for those specific scenarios and good be have? Thanks!

Add new tests convering types related to changes made under kubernetes#46

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 20, 2017

thockin reviewed Apr 20, 2017

View reviewed changes

spzala force-pushed the deepcopylintifelse branch from 71b8899 to 84c1873 Compare April 21, 2017 18:47

spzala force-pushed the deepcopylintifelse branch from 84c1873 to 671b501 Compare April 25, 2017 13:59

spzala force-pushed the deepcopylintifelse branch from 671b501 to a438c28 Compare April 27, 2017 20:03

thockin closed this Apr 27, 2017

thockin reopened this Apr 27, 2017

spzala closed this Apr 27, 2017

spzala reopened this Apr 27, 2017

thockin merged commit af11bb5 into kubernetes:master Apr 27, 2017

spzala added a commit to spzala/gengo that referenced this pull request Apr 30, 2017

Add new test for deepcopy

32938ba

Add new test convering servicecatalog types related to changes made under kubernetes#46

spzala mentioned this pull request Apr 30, 2017

Add new test for deepcopy #51

Merged

spzala added a commit to spzala/gengo that referenced this pull request May 2, 2017

Add new tests for deepcopy

4c78c60

Add new tests convering types related to changes made under kubernetes#46

spzala added a commit to spzala/gengo that referenced this pull request May 2, 2017

Add new tests for deepcopy

331d9bf

Add new tests convering types related to changes made under kubernetes#46

spzala mentioned this pull request May 4, 2017

Fix lint failure in deepcopy generated files kubernetes/kubernetes#41764

Closed

liggitt reviewed May 15, 2017

View reviewed changes

spzala added a commit to spzala/gengo that referenced this pull request May 23, 2017

Add new tests for deepcopy

aa0cba9

Add new tests convering types related to changes made under kubernetes#46

spzala added a commit to spzala/gengo that referenced this pull request May 23, 2017

Add new tests for deepcopy

a505af0

Add new tests convering types related to changes made under kubernetes#46

n1koo pushed a commit to n1koo/gengo that referenced this pull request Jan 19, 2018

Add new tests for deepcopy

d12f47b

Add new tests convering types related to changes made under kubernetes#46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix if loop related lint failures in the deepcopy-gen #46

Fix if loop related lint failures in the deepcopy-gen #46

spzala commented Apr 20, 2017

k8s-reviewable commented Apr 20, 2017

spzala commented Apr 20, 2017

thockin Apr 20, 2017

spzala Apr 21, 2017

thockin commented Apr 23, 2017

thockin commented Apr 23, 2017

thockin commented Apr 23, 2017

spzala commented Apr 23, 2017

thockin commented Apr 23, 2017

spzala commented Apr 23, 2017

thockin commented Apr 24, 2017

spzala commented Apr 24, 2017

thockin commented Apr 25, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017 via email

thockin commented Apr 27, 2017 via email

spzala commented Apr 27, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017

spzala commented Apr 27, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017 via email

spzala commented Apr 28, 2017

liggitt May 15, 2017 •

edited

Loading

liggitt May 15, 2017 •

edited

Loading

spzala May 15, 2017

liggitt May 15, 2017 •

edited

Loading

liggitt May 15, 2017

spzala May 15, 2017

spzala May 15, 2017

Fix if loop related lint failures in the deepcopy-gen #46

Fix if loop related lint failures in the deepcopy-gen #46

Conversation

spzala commented Apr 20, 2017

k8s-reviewable commented Apr 20, 2017

spzala commented Apr 20, 2017

thockin Apr 20, 2017

Choose a reason for hiding this comment

spzala Apr 21, 2017

Choose a reason for hiding this comment

thockin commented Apr 23, 2017

thockin commented Apr 23, 2017

thockin commented Apr 23, 2017

spzala commented Apr 23, 2017

thockin commented Apr 23, 2017

spzala commented Apr 23, 2017

thockin commented Apr 24, 2017

spzala commented Apr 24, 2017

thockin commented Apr 25, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017 via email

thockin commented Apr 27, 2017 via email

spzala commented Apr 27, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017

spzala commented Apr 27, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017

spzala commented Apr 27, 2017

thockin commented Apr 27, 2017 via email

spzala commented Apr 28, 2017

liggitt May 15, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt May 15, 2017 • edited Loading

Choose a reason for hiding this comment

spzala May 15, 2017

Choose a reason for hiding this comment

liggitt May 15, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt May 15, 2017

Choose a reason for hiding this comment

spzala May 15, 2017

Choose a reason for hiding this comment

spzala May 15, 2017

Choose a reason for hiding this comment

liggitt May 15, 2017 •

edited

Loading

liggitt May 15, 2017 •

edited

Loading

liggitt May 15, 2017 •

edited

Loading