-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix go vet error with go1.11 #954
Conversation
Since this affects only a test, why not just change the |
|
Oh, right... doh. It seems like we could come up with a solution that has less repetition... like taking advantage of the same logic in the ...
got, err := r.GetContent()
stringOrNil := func(s *string) string {
if s == nil {
return "<nil>"
}
return *s
}
encoding := stringOrNil(tt.encoding)
content := stringOrNil(tt.content)
... What do you think? |
We'll need to think about a better long term way to deal with gofmt changes. I think the best future-proof solution is to have the |
@gmlewis - changed to use |
github/repos_contents_test.go
Outdated
return *s | ||
} | ||
encoding := stringOrNil(tt.encoding) | ||
content := stringOrNil(tt.content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's appropriate to add this much code in between got, err := ...
and the if err != nil ...
statements while keeping the variable name just err
(see here). A future reader may think there's a missing error check after the got, err := ...
line.
I would suggest factoring out the definition of stringOrNil
to the very top of TestRepositoryContent_GetContent
, or even outside to the package scope, below TestRepositoryContent_GetContent
. And the stringOrNil(...)
calls can just be inlined into the corresponding t.Errorf
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, done with 2c7b329
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github/repos_contents_test.go
Outdated
if s == nil { | ||
return "<nil>" | ||
} | ||
return *s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random musing - what about making this line return fmt.Sprintf("%q", *s)
to preserve the quoting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want the function call to have quotes around the parameters as in a "real" one, I think we should add them to the t.Errorf
's format, and change the surrounding quotes markers:
if err == nil && tt.wantErr {
t.Errorf(`RepositoryContent("%s", "%s") did not return unexpected error`,
stringOrNil(tt.encoding), stringOrNil(tt.content))
}
- this would allow to keep stringOrNil
doing straightforward conversion to string or "<nil>" without adding additional formatting (surrounding quotes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not correct because <nil>
would then be quoted like "<nil>"
which is not right.
It should either be <nil>
or "string-value"
.
But no big deal... we can ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right; changed stringOrNil
to use fmt.Sprintf("%q", *s)
for quoting
github/repos_contents_test.go
Outdated
} | ||
if want := tt.want; got != want { | ||
t.Errorf("RepositoryContent.GetContent returned %+v, want %+v", got, want) | ||
} | ||
} | ||
} | ||
|
||
// stringOrNil converts a potentially null string pointer to string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add period to end of sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
83fed73
to
633f1e9
Compare
github/repos_contents_test.go - use helper function to display string or <nil> using string pointer. github/misc_test.go - add a line break to allow passing gofmt check with both go1.10 and go.11.
@gmlewis - could you merge this, or do you need the second approval? Thanks. |
@dmitris - Sorry for the delay. I think this is good-to-go and doesn't need a second LGTM now. |
currently builds of the package with go1.11 fail due to using
%q
qualifier with*string
type in github/repos_contents_test.go:In Travis, a number of PR builds fail with
Go:master
("Allowed failures"), ex. https://travis-ci.org/google/go-github/builds/406237120PR fixes the issue by treating the cases where tt.enconding and tt.content are nil or not nil separately and adding extra varaibles (
encoding
andcontent
) for printing in error messages. It also adds a workaround for gofmt difference between go1.10 and go1.11 (as in golang/go#26228, Go commit: golang/go@542ea5a) by adding an empty line.