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

Add option to define a package that will be imported #52

Merged

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Apr 1, 2022

Closes #48

Usage:

AWS_SDK_DIR=/Users/skarlso/go/pkg/mod/github.com/aws/aws-sdk-go-v2/service/sts@v1.16.3
./ifacemaker -f '/Users/skarlso/go/pkg/mod/github.com/aws/aws-sdk-go-v2/service/sts@v1.16.3/*.go' -s Client -i STS -p awsapi -y 'STS provides an interface to the AWS STS service.' -c 'Code generated by ifacemaker; DO NOT EDIT.' -m github.com/aws/aws-sdk-go-v2/service/sts

Produces:

// Code generated by ifacemaker; DO NOT EDIT.

package awsapi

import (
	"context"

	. "github.com/aws/aws-sdk-go-v2/service/sts"
)

// STS provides an interface to the AWS STS service.
type STS interface {
        // ... lots of comments
	AssumeRole(ctx context.Context, params *AssumeRoleInput, optFns ...func(*Options)) (*AssumeRoleOutput, error)

Note, that now *AssumeRoleInput works as expected. The import . "github.com/aws/aws-sdk-go-v2/service/sts" has been correctly added to the list of imports.

@Skarlso
Copy link
Contributor Author

Skarlso commented Apr 1, 2022

Running unit tests locally:

go test ./...
ok      github.com/vburenin/ifacemaker  0.490s
ok      github.com/vburenin/ifacemaker/maker    0.300s
➜  ifacemaker git:(fix_source_package_vs_target_package)

This change will allow us to drop using an extra script here: eksctl-io/eksctl#4994

If this is merged and released we can ditch our hack around missing imports.

We would greatly appreciate that. :) thanks!

@vburenin vburenin merged commit b2018d8 into vburenin:master Apr 2, 2022
@vburenin
Copy link
Owner

vburenin commented Apr 2, 2022

whatever... if it helps %-)

@Skarlso
Copy link
Contributor Author

Skarlso commented Apr 2, 2022

Best comment ever. 😂 Thank you, it does. 😊

@vburenin
Copy link
Owner

vburenin commented Apr 2, 2022

I actually not exactly agree with this. Dot imports are really bad, I simply have no time and capability to build and test this, so I bet on the community to do the right thing. Make sure you don't shoot yourself into the foot.

@vburenin
Copy link
Owner

vburenin commented Apr 2, 2022

Is this one actually better? https://github.com/rjeczalik/interfaces

@Skarlso
Copy link
Contributor Author

Skarlso commented Apr 3, 2022

@vburenin Why are dot imports bad? They are a language feature. And you have to define them explicitly, so there shouldn't be too much a problem with them. If you don't set this, you won't get into trouble. Although what trouble do you envision?

If it's the same package you import, you should be doing okay. :) It would be a lot harder to determine package local declarations and alias them with a random prefix. 🤔

@vburenin
Copy link
Owner

vburenin commented Apr 3, 2022 via email

@Skarlso
Copy link
Contributor Author

Skarlso commented Apr 3, 2022

This argument doesn't hold since you already are in the package you are importing into the target package. You know the target package. Of course if the target package has the same naming as the source package that will be a problem. But in that case don't do this and you should be fine. That said, I understand what you are saying that's why it's an option explicitly defined by the user. The alternative would be to give an optional alias and prepend all package local variables.

@vburenin
Copy link
Owner

vburenin commented Apr 3, 2022 via email

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.

Source package not imported if interfaces placed in different package
2 participants