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

Fix replace-type for different packages from the same source #710

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

RangelReale
Copy link
Contributor

@RangelReale RangelReale commented Sep 23, 2023

Description

When different packages that map to the same source package and mapped with replace-type, the package of the first one are used for all other packages.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • 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
  • 1.19
  • 1.20
  • 1.21

How Has This Been Tested?

  • a test was added for this specific problem.
  • @rkoehl05 was able to validate the solution in his mock repo.

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

@RangelReale
Copy link
Contributor Author

@LandonTClipp had to add the type to the addPackageImportWithName to be able to differentiate between them, I only replace if this parameter is not nil. Also added some specifity rule to check replaces with types first.

@rkoehl05 can you see if this fixes your problem?

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f54eea9) 74.88090% compared to head (ee33b3d) 74.91349%.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master        #710         +/-   ##
===================================================
+ Coverage   74.88090%   74.91349%   +0.03258%     
===================================================
  Files              9           9                 
  Lines           2309        2312          +3     
===================================================
+ Hits            1729        1732          +3     
  Misses           442         442                 
  Partials         138         138                 
Files Coverage Δ
pkg/generator.go 93.34171% <100.00000%> (+0.02519%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rkoehl05
Copy link

Thanks @RangelReale! I tested with my reproducer and I was able to generate a working mock.

@LandonTClipp LandonTClipp marked this pull request as draft October 4, 2023 17:31
@LandonTClipp
Copy link
Collaborator

@RangelReale thanks so much for this. I converted this to a draft because you mentioned this is still a WIP. Please ping me when you're ready for me to review.

@RangelReale
Copy link
Contributor Author

@RangelReale thanks so much for this. I converted this to a draft because you mentioned this is still a WIP. Please ping me when you're ready for me to review.

@LandonTClipp I updated the description, it should be ready now.

@LandonTClipp LandonTClipp marked this pull request as ready for review October 4, 2023 19:35
Copy link
Collaborator

@LandonTClipp LandonTClipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good implementation. Just minor nitpicks about the naming that could make this a bit clearer.

type Fiz interface {
FA(f fiz1.Fiz1)
FB(f fiz2.Fiz2)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would request you add a short README.md to example_project/fiz explaining what this package is testing.

Also I'd like it to be renamed to something more descriptive like replaceType or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed the files and types, and added a README.

if !f(item.from, item.to) {
break
// check most specific first
for _, hasType := range []bool{true, false} {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good solution 👍🏻

Copy link
Collaborator

@LandonTClipp LandonTClipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thank you!

@LandonTClipp
Copy link
Collaborator

@RangelReale if you could squash the commits down to one, we can merge away.

- check more specific replace types first
@RangelReale
Copy link
Contributor Author

@RangelReale if you could squash the commits down to one, we can merge away.

rebased to master, see if this is ok.

@LandonTClipp LandonTClipp merged commit 246df9d into vektra:master Oct 5, 2023
5 checks passed
@LandonTClipp
Copy link
Collaborator

Thank you very much for getting this out. I lean heavily on people submitting PRs ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--replace-type is not mapping packages correctly
3 participants