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.

Sign up for GitHub

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

Jump to bottom

feat: goctl logic tpl add request type data #1705

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: goctl logic tpl add request type data #1705

wants to merge 2 commits into from

Conversation

re-dylan
Copy link
Collaborator

No description provided.

@re-dylan
Copy link
Collaborator Author

目前存在的问题，

  1. hasRequest 与 handler 中的 HasRequest 命名不够统一。
  2. respType 返回的是指针类型，目前没有处理 responseGoTypeName 中的返回类型。

@kevwan kevwan requested review from kevwan and kesonan March 24, 2022 09:32
kevwan
kevwan reviewed Mar 24, 2022
View reviewed changes
tools/goctl/api/gogen/genlogic.go
@@ -34,10 +34,10 @@ func New{{.logic}}(ctx context.Context, svcCtx *svc.ServiceContext) *{{.logic}}
}
}

func (l *{{.logic}}) {{.function}}({{.request}}) {{.responseType}} {
func (l *{{.logic}}) {{.function}}({{if .hasRequest}}req *{{.reqType}}{{end}}) {{if .hasResp}}(resp {{.respType}}, err error){{else}}(err error){{end}} {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if only returns error, just use error before {

tools/goctl/api/gogen/genlogic.go
"responseType": responseString,
"returnString": returnString,
"request": requestString,
"responseType": responseString, // DEPRECATED: the responseType will be remove
Copy link
Contributor

Choose a reason for hiding this comment

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

remove these deprecated lines directly?

@kevwan kevwan added kind/need-more-discussion Not decided, need more discussion! do-not-merge/hold Indicates that a PR should not merge because of known issues or need more discussion. labels Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevwan kevwan kevwan left review comments

@kesonan kesonan Awaiting requested review from kesonan

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because of known issues or need more discussion. kind/need-more-discussion Not decided, need more discussion!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@re-dylan @kevwan