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

feat(hz): optional filed parse for client #1177

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

Conversation

FGYFFFF
Copy link
Contributor

@FGYFFFF FGYFFFF commented Aug 26, 2024

What type of PR is this?

feat

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo
    针对 hz client,对 optional 字段进行处理,如果用户不设置对应值,则不传这个值

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en: optional filed parse for client
zh(optional):

(Optional) Which issue(s) this PR fixes:

(Optional) The PR that updates user documentation:

@FGYFFFF FGYFFFF requested review from a team as code owners August 26, 2024 09:22
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.86%. Comparing base (dbd3166) to head (122001c).
Report is 10 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1177      +/-   ##
===========================================
+ Coverage    82.61%   82.86%   +0.25%     
===========================================
  Files          123      124       +1     
  Lines        11455    11695     +240     
===========================================
+ Hits          9463     9691     +228     
- Misses        1419     1431      +12     
  Partials       573      573              
Flag Coverage Δ
82.86% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -194,6 +194,9 @@ func Init() *cli.App {
handlerByMethod := cli.BoolFlag{Name: "handler_by_method", Usage: "Generate a separate handler file for each method.", Destination: &globalArgs.HandlerByMethod}
trimGoPackage := cli.StringFlag{Name: "trim_gopackage", Aliases: []string{"trim_pkg"}, Usage: "Trim the prefix of go_package for protobuf.", Destination: &globalArgs.TrimGoPackage}

// client flag
enableClientOptionalFlag := cli.BoolFlag{Name: "enable_optional", Usage: "Optional field do not transfer for thrift if not set.(Only works for query tag)", Destination: &globalArgs.EnableClientOptional}
Copy link
Member

Choose a reason for hiding this comment

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

要在名称里指出来 Query 吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个不仅仅是 query 场景,可能后续的 form 场景也会有;等用户有需求的时候我再form 场景也给加上

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我这次都给加上吧,要不之后再加就不好加了

@@ -194,6 +194,9 @@ func Init() *cli.App {
handlerByMethod := cli.BoolFlag{Name: "handler_by_method", Usage: "Generate a separate handler file for each method.", Destination: &globalArgs.HandlerByMethod}
trimGoPackage := cli.StringFlag{Name: "trim_gopackage", Aliases: []string{"trim_pkg"}, Usage: "Trim the prefix of go_package for protobuf.", Destination: &globalArgs.TrimGoPackage}

// client flag
enableClientOptionalFlag := cli.BoolFlag{Name: "enable_optional", Usage: "Optional field do not transfer for thrift if not set.(Only works for query tag)", Destination: &globalArgs.EnableClientOptional}
Copy link
Member

Choose a reason for hiding this comment

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

flag 的名字和描述里也说明下是 Client 的?另外这个行为默认enable 有啥问题吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok。这个会对之前的行为有影响,导致之前传值现在不传值;目前来看,有这个需求的用户还是比较少的,所以还是单独搞一个 flag 出来

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants