Skip to content
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

Removed unused package import logic #490

Merged
merged 1 commit into from Nov 15, 2022

Conversation

meshuga
Copy link
Contributor

@meshuga meshuga commented Jul 2, 2022

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/tools@v0.1.11/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:

Type of change

  • 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
  • 1.18

How Has This Been Tested?

As above.

Checklist

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2022

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%) ⬆️

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.

@LandonTClipp
Copy link
Contributor

LandonTClipp commented Jul 15, 2022

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.

@meshuga
Copy link
Contributor Author

meshuga commented Jul 29, 2022

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 :)

@LandonTClipp LandonTClipp merged commit 847d988 into vektra:master Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants