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

improve example cases for package gi18n #2970

Merged
merged 5 commits into from
Sep 19, 2023
Merged

improve example cases for package gi18n #2970

merged 5 commits into from
Sep 19, 2023

Conversation

hailaz
Copy link
Member

@hailaz hailaz commented Sep 15, 2023

gvalid的单测和示例混着部分隐式使用i18n的情况,而且这个i18n来自util/gvalid/i18n文件夹,不太规范。
现在移除util/gvalid/i18n文件夹使用util/gvalid/testdata/i18n中的数据,两个文件夹的内容基本重复。

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.14% 🎉

Comparison is base (395df94) 79.09% compared to head (46f8ac7) 79.24%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2970      +/-   ##
==========================================
+ Coverage   79.09%   79.24%   +0.14%     
==========================================
  Files         643      643              
  Lines       52754    52754              
==========================================
+ Hits        41727    41806      +79     
+ Misses       8945     8867      -78     
+ Partials     2082     2081       -1     
Flag Coverage Δ
go-1.18-386 79.27% <100.00%> (+0.10%) ⬆️
go-1.18-amd64 79.27% <100.00%> (+0.10%) ⬆️
go-1.19-386 79.21% <100.00%> (+0.14%) ⬆️
go-1.19-amd64 79.16% <100.00%> (+0.11%) ⬆️
go-1.20-386 79.33% <100.00%> (+0.28%) ⬆️
go-1.20-amd64 79.25% <100.00%> (+0.09%) ⬆️
go-1.21-386 79.23% <100.00%> (+0.08%) ⬆️
go-1.21-amd64 79.33% <100.00%> (+0.23%) ⬆️

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

Files Changed Coverage Δ
util/gvalid/internal/builtin/builtin_password.go 100.00% <100.00%> (+18.18%) ⬆️
util/gvalid/internal/builtin/builtin_password2.go 100.00% <100.00%> (+13.33%) ⬆️
util/gvalid/internal/builtin/builtin_password3.go 100.00% <100.00%> (+12.50%) ⬆️

... and 40 files with indirect coverage changes

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

@gqcn
Copy link
Member

gqcn commented Sep 18, 2023

@hailaz 单例和示例代码没必要严格解耦,特别是示例的代码是你的代码写什么样别人看到就是什么样,假如你做了封装了其实别人看示例代码会很懵的。比如:
image

@gqcn gqcn closed this Sep 18, 2023
@gqcn gqcn added the rejected The proposal or PR is not accepted, which might be conflicted with our design or plan. label Sep 18, 2023
@hailaz
Copy link
Member Author

hailaz commented Sep 19, 2023

@hailaz 单例和示例代码没必要严格解耦,特别是示例的代码是你的代码写什么样别人看到就是什么样,假如你做了封装了其实别人看示例代码会很懵的。比如: image

那就统一示例吧,现在是有些返回信息是指定了i18n的信息,有些返回了默认信息。然后这个i18n的目录文件其实和testdata中的i18n是重复的,而且本该放到testdata中。

@hailaz hailaz reopened this Sep 19, 2023
@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@hailaz There is no need to strictly decouple the singleton and the sample code. Especially the sample code is what others see when you write the code. If you encapsulate it, others will be confused when they see the sample code. For example: image

Let’s unify the examples. Now some of the returned information specifies i18n information, and some return default information. Then this i18n directory file is actually duplicated with the i18n in testdata, and should have been placed in testdata.

@hailaz hailaz removed the rejected The proposal or PR is not accepted, which might be conflicted with our design or plan. label Sep 19, 2023
@gqcn gqcn changed the title gvalid的单测和示例混着部分隐式使用i18n的情况,这部分改为显式使用i18n improve example cases for package gi18n Sep 19, 2023
@gqcn gqcn merged commit 42de1c1 into master Sep 19, 2023
24 checks passed
@gqcn gqcn deleted the fix/gvalid-i18n branch September 19, 2023 12:18
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.

4 participants