build: TestProtoMarshal correctly avoids Marshal methods #18496
Conversation
|
This change is |
|
This is on my radar! Just still catching up from vacation. |
|
Rebased to fix a merge conflict. |
|
@CLAassistant where you at? Reviewed 20 of 20 files at r1. build/style_test.go, line 139 at r1 (raw file):
Where does a layman find documentation on this Comments from Reviewable |
|
Good question, I think I got it from some SO thread. Thanks!
…On Sep 20, 2017 01:08, "Nathan VanBenschoten" ***@***.***> wrote:
[image: <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/>]
<https://camo.githubusercontent.com/41b3ec3419116e3b3f84507ad965646ce4ce1f0c/68747470733a2f2f72657669657761626c652e696f2f6c67746d2e706e67>
@CLAassistant <https://github.com/claassistant> where you at?
------------------------------
Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved
discussion, all commit checks successful.
------------------------------
*build/style_test.go, line 139 at r1
<https://reviewable.io:443/reviews/cockroachdb/cockroach/18496#-KuSbJWc9Fp3NCPphJuy:-KuSbJWc9Fp3NCPphJuz:bs474mn>
(raw file
<https://github.com/cockroachdb/cockroach/blob/667ce2d2138bcc0c334d4bc99953b414c280f438/build/style_test.go#L139>):*
re: `\bos\.(Getenv|LookupEnv)\(`,
excludes: []string{
":!acceptance",
Where does a layman find documentation on this :! exclusion syntax?
------------------------------
*Comments from Reviewable
<https://reviewable.io:443/reviews/cockroachdb/cockroach/18496#-:-KuScng906XHgkeEU5YK:b-8al1yx>*
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#18496 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPNHg7Mqw6JCq543uPmcKXCF72LWDks5skJ3jgaJpZM4PYDE6>
.
|
Also use `git grep` exclusions rather than filtering its results.
|
Rebased to avoid skew; looks like acceptance hung, and my teamcity privileges have been revoked :( Can someone rebuild for me? |
|
Queued you a new build! Review status: 19 of 20 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending. build/style_test.go, line 139 at r1 (raw file): Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
https://git-scm.com/docs/gitglossary/2.13.1#gitglossary-aiddefpathspecapathspec (Git pathspecs are super powerful.) Comments from Reviewable |
| re: `\bos\.(Getenv|LookupEnv)\(`, | ||
| excludes: []string{ | ||
| ":!acceptance", | ||
| ":!build/style_test.go", |
There was a problem hiding this comment.
This isn't in pkg, is it? Why would it get picked up?
There was a problem hiding this comment.
I think git grep doesn't care about the directory you're in, or at least it doesn't when you specify '*.go'.
| "*.go", | ||
| ":!util/protoutil/marshal.go", | ||
| ":!settings/settings_test.go", | ||
| ) |
There was a problem hiding this comment.
This pattern really seems to deserve a grepExcluding(regexpattern, "file", "file2", ...) function, but this is an improvement as is so no need to hold up this PR.
There was a problem hiding this comment.
Eh, I'm not sure I agree that there's much benefit to the indirection. Feel free to follow up if you feel strongly enough.
| @@ -649,6 +727,9 @@ func TestStyle(t *testing.T) { | |||
| // TODO(tamird): replace this with errcheck.NewChecker() when | |||
| // https://github.com/dominikh/go-tools/issues/57 is fixed. | |||
| t.Run("TestErrCheck", func(t *testing.T) { | |||
| if testing.Short() { | |||
| t.Skip("short flag") | |||
| } | |||
There was a problem hiding this comment.
Any particular reason you're sidecar-ing this bit?
There was a problem hiding this comment.
I think this just got missed earlier; returncheck is roughly the same thing and it has this logic.
Also use
git grepexclusions rather than filtering its results.