vburenin / ifacemaker

Generate interfaces from structure methods.
Apache License 2.0
320 stars 43 forks source link

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

Closed Skarlso closed 2 years ago

Skarlso commented 2 years ago

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 commented 2 years ago

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: https://github.com/weaveworks/eksctl/issues/4994

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

We would greatly appreciate that. :) thanks!

vburenin commented 2 years ago

whatever... if it helps %-)

Skarlso commented 2 years ago

Best comment ever. πŸ˜‚ Thank you, it does. 😊

vburenin commented 2 years ago

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 commented 2 years ago

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

Skarlso commented 2 years ago

@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 commented 2 years ago

Imports into the package space are considered bad practices in any programming language I am aware of except C where it is just life. There is a chance of the name collisions and it makes harder to distinguish between local package definitions and the package level. Having a language feature doesn’t mean it is a good thing.

On Sun, Apr 3, 2022 at 2:40 AM Gergely Brautigam @.***> wrote:

@vburenin https://github.com/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. :)

β€” Reply to this email directly, view it on GitHub https://github.com/vburenin/ifacemaker/pull/52#issuecomment-1086795739, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBGHK2G3ZFM3WFGBCDIRFTVDFDM7ANCNFSM5SHYGCVQ . You are receiving this because you were mentioned.Message ID: @.***>

Skarlso commented 2 years ago

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 commented 2 years ago

I have no incentive to convince anybody. Do it the way you think it is best for you.

On Sun, Apr 3, 2022 at 11:06 AM Gergely Brautigam @.***> wrote:

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.

β€” Reply to this email directly, view it on GitHub https://github.com/vburenin/ifacemaker/pull/52#issuecomment-1086898406, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBGHK5J2DOK7422J36B37DVDG6XBANCNFSM5SHYGCVQ . You are receiving this because you were mentioned.Message ID: @.***>