mockery icon indicating copy to clipboard operation
mockery copied to clipboard

Removed unused package import logic

Open meshuga opened this issue 2 years ago • 3 comments

Description

@LandonTClipp, I initially thought the current code to build imports list has special logic for vendored files. To prove it, I created a below fixture file:

package test

import (
	"github.com/rs/zerolog"
)

type ImportsVendored interface {
	A() zerolog.Logger
}

And a test:

func (s *GeneratorSuite) TestPrologueWithImportVendored() {
	generator := s.getGenerator(
		"imports_vendored.go", "ImportsVendored", false, "",
	)
	expected := `package mocks

import mock "github.com/stretchr/testify/mock"
import test "github.com/vektra/mockery/v2/pkg/fixtures"
import zerolog "github.com/rs/zerolog"

`

	s.checkPrologueGeneration(generator, expected)
}

Moved libs to vendor, to see how vendored dependencies will be handled: image

I tried different approaches and did go deeper into the x/tools but I can't make the Go SDK return a file location, it's always Go package path: image

The https://pkg.go.dev/golang.org/x/[email protected]/go/packages library that's used to analyze an interface and fetch used types, relies on Go import semantics. There are no references to actual files on disk that are being used by mockery.

Why would there be any references to a file disk? 6 years ago, the tool was actually traversing the GOPATH paths to look for files to scan. The original author had the code that would transform file references to Go import packages aka. localize them. That wasn't perfect, so that’s why issues like #116 were created, with respective fixes.

Because the project doesn't traverse GOPATH anymore but uses only Go import abstractions from x/tools, there's no reason to keep that code anymore.

I marked this PR as bugfix as it removes code that is not used anymore, it only fixes the below issues:

  • Fixes #419
  • Fixes #249
  • Fixes #481

Type of change

  • [x] Bug fix (non-breaking change which fixes an issue), see comment below
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

Version of Golang used when building/testing:

  • [ ] 1.11
  • [ ] 1.12
  • [ ] 1.13
  • [ ] 1.14
  • [ ] 1.15
  • [ ] 1.16
  • [ ] 1.17
  • [x] 1.18

How Has This Been Tested?

As above.

Checklist

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes

meshuga avatar Jul 02 '22 10:07 meshuga

Codecov Report

Merging #490 (c82f3c3) into master (4d1f925) will decrease coverage by 0.76%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
- Coverage   72.42%   71.66%   -0.77%     
==========================================
  Files           6        6              
  Lines        1262     1214      -48     
==========================================
- Hits          914      870      -44     
+ Misses        304      301       -3     
+ Partials       44       43       -1     
Impacted Files Coverage Δ
pkg/generator.go 93.93% <ø> (+0.15%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4d1f925...c82f3c3. Read the comment docs.

codecov-commenter avatar Jul 02 '22 10:07 codecov-commenter

That's a great find, I am pretty busy over the next few days but if we can axe this logic completely that would be great! I was operating under the assumption that this logic was still needed but honestly I didn't really test that. I think you've shown it's not needed.

LandonTClipp avatar Jul 15 '22 19:07 LandonTClipp

That's a great find, I am pretty busy over the next few days but if we can axe this logic completely that would be great! I was operating under the assumption that this logic was still needed but honestly I didn't really test that. I think you've shown it's not needed.

Thanks! Happy to see it merged :)

meshuga avatar Jul 29 '22 16:07 meshuga