Check unvendored version of package paths for ignore parameter #109
Conversation
| // does not represent a vendored path). The second return value is true if the provided package was vendored, false | ||
| // otherwise. | ||
| func nonVendoredPkgPath(pkgPath string) (string, bool) { | ||
| lastVendorIndex := strings.LastIndex(pkgPath, "/vendor/") |
There was a problem hiding this comment.
I think the path-based approach here is reasonable given that core Go code does something similar: https://github.com/golang/go/blob/81dfcba331f43bd14c8933eca83c433e53cb7b55/src/cmd/go/build.go#L2324
The other approach would be to iterate over all of the v.ignore keys and try to import them and see if they resolve to a vendored package, but this requires more information (need a source directory from which to try to resolve import), so I think this approach is reasonable. However, would be happy to hear other feedback.
| // if current package being considered is vendored, check to see if it should be ignored based | ||
| // on the unvendored path. | ||
| if nonVendoredPkg, ok := nonVendoredPkgPath(pkg.Path()); ok { | ||
| if re, ok := v.ignore[nonVendoredPkg]; ok { |
There was a problem hiding this comment.
Adding this as a second check so that any existing ignore entries that use fully qualified vendor paths will still work just as they did before. This extra check basically just "unvendors" the package being considered and uses the ignore entry there.
I think this is the correct behavior and reflects the intent of #102.
|
|
||
| func TestIgnore(t *testing.T) { | ||
| // copy testvendor directory into current directory for test | ||
| if err := os.MkdirAll("testvendor", 0755); err != nil { |
There was a problem hiding this comment.
Writing out files directly in test because it's not possible to test vendor behavior within the testdata directory because Go hard-codes behavior that doesn't resolve vendored imports within a testdata directory.
nmiyake
force-pushed
the
fix102
branch
2 times, most recently
from
1b6dc98
to
fbf65d9
Compare
| "testing" | ||
| ) | ||
|
|
||
| const testPackage = "github.com/kisielk/errcheck/testdata" | ||
| const ( |
There was a problem hiding this comment.
Just as a matter of style, I don't think the const blocks are necessary
| @@ -83,6 +90,94 @@ func TestAll(t *testing.T) { | |||
| test(t, CheckAsserts|CheckBlank) | |||
| } | |||
|
|
|||
| const ( | |||
There was a problem hiding this comment.
ditto here, since it's just one constant
There was a problem hiding this comment.
Actually 2 constants here (testVendorMain and testLog), but don't feel strongly, so removed block.
| t.SkipNow() | ||
| } | ||
|
|
||
| // copy testvendor directory into current directory for test |
There was a problem hiding this comment.
I would prefer to use ioutil.TempDir for this.
|
Updated PR to address feedback. |
|
Thanks for the review! Makes sense to have others test as well -- is there an existing process for this? Happy to try to do anything to help this out on my end. |
|
Well, you've already pinged a couple of people, hopefully they can try out the changes and give some feedback 👍 |
|
OK, I tested this on the the 2 major projects that @bmoylan and I work on, and also tried it on the project that @dtolnay works on (all of these projects have pre-existing verification scripts that use
Based on these tests and the logic of the change, I'm pretty confident that it should work as intended. |
|
Sounds good, let's see how it goes :) |
Fixes #102