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(goctl): multi imports the api cause redeclared error in types.go #3988

Merged
merged 4 commits into from Apr 4, 2024

Conversation

wjiec
Copy link
Contributor

@wjiec wjiec commented Mar 8, 2024

Now experimental features are turned on by default in Goctl, is this correct?

if !goctlEnv.HasKey(GoctlExperimental) {
goctlEnv.SetKV(GoctlExperimental, ExperimentalOn)
}

@kesonan
Copy link
Collaborator

kesonan commented Mar 9, 2024

Plz describe which issue scenario should be fix

@wjiec
Copy link
Contributor Author

wjiec commented Mar 9, 2024

Hi @kesonan, you can check the added test cases to understand that the specific scenario is in a gateway that has the following referenced relationship:

// main.api
import "xxx.api"
import "yyy.api"

// xxx.api
import "common.api"

// yyy.api
import "common.api"

Use goctl api go -api main.api ... generated types.go has redeclared structs (from common.api).

@wjiec wjiec force-pushed the bugfix/import-twice branch 2 times, most recently from e703b9d to 93ef7e4 Compare March 12, 2024 07:45
@wjiec
Copy link
Contributor Author

wjiec commented Mar 12, 2024

hi @kesonan @kevwan do you have time to look at this pr?

@kesonan
Copy link
Collaborator

kesonan commented Mar 27, 2024

image
The master branch has already fix this issue, pls try with latest version.

@wjiec
Copy link
Contributor Author

wjiec commented Mar 27, 2024

Hi @kesonan , I still have this problem with the latest code in master branch, please check the contents of the internal/types/types.go file. Screenshots:

image

@wjiec
Copy link
Contributor Author

wjiec commented Mar 27, 2024

Sorry, it should be that the title of the PR is not expressed clearly enough, and the redeclared error appears in the types.go file.

@wjiec wjiec changed the title fix(goctl): multi imports the api cause redeclared error fix(goctl): multi imports the api cause redeclared error in types.go Mar 27, 2024
@kevwan
Copy link
Contributor

kevwan commented Mar 30, 2024

Is the problem fixed already?

@wjiec
Copy link
Contributor Author

wjiec commented Mar 30, 2024

@kevwan Unfortunately the code on master still has this problem now, and this pr is designed to fix it.

@wjiec wjiec force-pushed the bugfix/import-twice branch 2 times, most recently from 02a1f8d to bfc3018 Compare March 30, 2024 05:54
Copy link

codecov bot commented Mar 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.16%. Comparing base (2629636) to head (28ae09c).
Report is 47 commits behind head on master.

Additional details and impacted files

see 254 files with indirect coverage changes

@kesonan
Copy link
Collaborator

kesonan commented Mar 30, 2024

pls execute the following cmd goctl env and post the output here, now it is already fixed on master.

@wjiec
Copy link
Contributor Author

wjiec commented Mar 30, 2024

@kesonan Okay, this is the output of the goctl env command

GOCTL_OS=darwin
GOCTL_ARCH=arm64
GOCTL_HOME=/Users/jayson/.goctl
GOCTL_DEBUG=False
GOCTL_CACHE=/Users/jayson/.goctl/cache
GOCTL_EXPERIMENTAL=on
GOCTL_VERSION=1.6.3
PROTOC_VERSION=
PROTOC_GEN_GO_VERSION=v1.33.0
PROTO_GEN_GO_GRPC_VERSION=1.3.0

btw, something that could potentially have an impact on this is the value of the GOCTL_EXPERIMENTAL env, by default, the value of this environment variable is set to On, I'm guessing this is set to Off by default in your environment?

@wjiec
Copy link
Contributor Author

wjiec commented Mar 30, 2024

I retested with the latest code on master, as well as executing goctl api go ... on the following envs. and the result is still wrong.

$ env | grep GOCTL
# nothing
$ ./goctl env
GOCTL_OS=darwin
GOCTL_ARCH=arm64
GOCTL_HOME=/Users/jayson/.goctl
GOCTL_DEBUG=False
GOCTL_CACHE=/Users/jayson/.goctl/cache
GOCTL_EXPERIMENTAL=on
GOCTL_VERSION=1.6.3
PROTOC_VERSION=
PROTOC_GEN_GO_VERSION=v1.33.0
PROTO_GEN_GO_GRPC_VERSION=1.3.0

@wjiec
Copy link
Contributor Author

wjiec commented Mar 30, 2024

Yes, after I executed . /goctl env -w GOCTL_EXPERIMENTAL=off, the generated types.go is now correct!

@kevwan
Copy link
Contributor

kevwan commented Apr 1, 2024

@kesonan PTAL.

@zeromicro zeromicro deleted a comment from Issues-translate-bot Apr 1, 2024
@kesonan
Copy link
Collaborator

kesonan commented Apr 3, 2024

@wjiec Please add my WeChat account and we can communicate on WeChat. The efficiency here is a bit low. WeChat ID is KesonAnn.

@wjiec wjiec force-pushed the bugfix/import-twice branch 2 times, most recently from 7ff7fb2 to 9bbf16f Compare April 3, 2024 14:56
@kevwan kevwan linked an issue Apr 3, 2024 that may be closed by this pull request
Copy link
Collaborator

@kesonan kesonan left a comment

Choose a reason for hiding this comment

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

LGTM

@kevwan kevwan added this pull request to the merge queue Apr 4, 2024
Merged via the queue into zeromicro:master with commit f138cc7 Apr 4, 2024
6 checks passed
@wjiec wjiec deleted the bugfix/import-twice branch April 4, 2024 12:29
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.

goctl generated duplicate api types.
3 participants