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

[WIP] allow imported constraints for generics #198

Closed
wants to merge 10 commits into from

Conversation

cbaker
Copy link
Contributor

@cbaker cbaker commented May 19, 2023

I'm not the most knowledgeable on the STDLIB given it's pretty big but this seems to work relatively well. πŸ€·β€β™‚οΈ
If there is a way to accomplish the helper functions more optimally, I'd love to know. Thanks!

Example of the problem:

package myotherpackage

import (
 "os/fs"
)

type T interface {
  *fs.FileInfo
}
package mypackage

import (
  "github.com/matryer/moq/myotherpackage"
)

type MyInterface[T myotherpackage.T] interface {
  M(T) error
}

If we try to put this into mock as it stands it will attempt to generate something like:

var _ MyInterfaceMock[*os/fs.FileInfo] = .....

This is a problem and causes moq to fail in 2 ways:

  1. Mocks will not generate because gofmt fails.
  2. We will generate a bad mock if gofmt were to not fail.

The solution I have presented will add "os/fs" to the imports on the mock and generate:

var _ MyInterfaceMock[*fs.FileInfo] = ....

@cbaker
Copy link
Contributor Author

cbaker commented May 19, 2023

Believe this will address:
#177
#190

Maybe: #173 ? Seems kinda "generic" ha

@cbaker cbaker changed the title allow external constraints for generic interfaces allow imported constraints for generic interfaces May 19, 2023
@cbaker cbaker changed the title allow imported constraints for generic interfaces allow imported constraints for generics May 23, 2023
@TheFellow
Copy link

@cbaker (and whoever else comes across this) I just built from your fork and it does not appear to fix #177 sadly. I brought over the failing test from PR #190, fixed up the golden file and confirmed it does not generate the imported generic's package when generating the code.

I built from your branch and sanity checked on my production use case and sadly confirmed it there as well. Haven't dug into it yet to try and fix it but I really need to understand it so I can try and get it sorted out. It's causing a headache and ugly workarounds that I want to stop from growing.

@cbaker
Copy link
Contributor Author

cbaker commented May 26, 2023

@cbaker (and whoever else comes across this) I just built from your fork and it does not appear to fix #177 sadly. I brought over the failing test from PR #190, fixed up the golden file and confirmed it does not generate the imported generic's package when generating the code.

I built from your branch and sanity checked on my production use case and sadly confirmed it there as well. Haven't dug into it yet to try and fix it but I really need to understand it so I can try and get it sorted out. It's causing a headache and ugly workarounds that I want to stop from growing.

Did you use the branch on my fork for this PR or main? This isn't merged into main on my fork yet.
Have you checked out the test cases I wrote in this PR to see if this is relevant to what you're looking to fix? I've tested it on some code at my work that was previously failing and it successfully generated a mock. Send me a failing test case and I will assist in looking into it. Thanks!

@TheFellow
Copy link

@cbaker That'd be amazing!

I haven't looked through your tests yet, but I just forked your repo and added the test case I tried to your branch. Added a PR back to your branch here.

@cbaker
Copy link
Contributor Author

cbaker commented May 26, 2023

@cbaker That'd be amazing!

I haven't looked through your tests yet, but I just forked your repo and added the test case I tried to your branch. Added a PR back to your branch here.

I think I see the problem. You're using a struct instead of an interface, hopefully simple to remedy, I'll check it out later tomorrow. Thanks.

@cbaker
Copy link
Contributor Author

cbaker commented Jun 2, 2023

@TheFellow I have replicated the issue and began working a solution, just have not had a crazy amount of time recently.

@cbaker cbaker changed the title allow imported constraints for generics [WIP] allow imported constraints for generics Jun 14, 2023
@TimVosch
Copy link
Contributor

Believe this will address: #177 #190

Maybe: #173 ? Seems kinda "generic" ha

I just opened PR #199 which fixes #177, might be relevant here.

@cbaker cbaker closed this Jun 20, 2023
@cbaker
Copy link
Contributor Author

cbaker commented Jun 20, 2023

Believe this will address: #177 #190
Maybe: #173 ? Seems kinda "generic" ha

I just opened PR #199 which fixes #177, might be relevant here.

I believe what you did will more than likely fix it for everyone in a way more eloquent manner. Great job. Closing this PR.

@TimVosch
Copy link
Contributor

TimVosch commented Jun 20, 2023 via email

@TheFellow
Copy link

@TimVosch I cloned your fork, built from your PR branch and tested it against our production scenario (a generic closing over an imported struct) and it worked perfectly! Thank you!

cc @cbaker

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.

3 participants