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

perf: improve paragragh performance #35

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

rfyiamcool
Copy link
Contributor

summary

improve paragragh performance.

before optimize

goos: darwin
goarch: amd64
pkg: github.com/go-faker/faker/v4
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkParagraph
BenchmarkParagraph-12    	   21318	     50666 ns/op	   20836 B/op	     164 allocs/op
PASS
ok  	github.com/go-faker/faker/v4	1.833s

after optimize

goos: darwin
goarch: amd64
pkg: github.com/go-faker/faker/v4
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkParagraph
BenchmarkParagraph-12    	   25045	     43137 ns/op	   17784 B/op	      94 allocs/op
PASS
ok  	github.com/go-faker/faker/v4	1.867s

Signed-off-by: rfyiamcool <rfyiamcool@163.com>
lorem_test.go Outdated
@@ -52,13 +52,15 @@ func TestFakeWord(t *testing.T) {

func TestFakeSentence(t *testing.T) {
res := Sentence()
fmt.Println(res)
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this print please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lorem_test.go Outdated
if res == "" || !strings.HasSuffix(res, ".") {
t.Error("Expected sentence")
}
}

func TestFakeParagraph(t *testing.T) {
res := Paragraph()
fmt.Println(res)
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During the test, the results were printed to see if they were as expected. When I submitted the PR, I forgot to delete it. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can remove this please?

done

Copy link

Choose a reason for hiding this comment

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

I like to use t.Log() here.

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

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

Comparison is base (f70bf73) 89.04% compared to head (f72acc5) 89.16%.
Report is 3 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   89.04%   89.16%   +0.11%     
==========================================
  Files          12       12              
  Lines        1762     1781      +19     
==========================================
+ Hits         1569     1588      +19     
  Misses        138      138              
  Partials       55       55              
Files Changed Coverage Δ
lorem.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

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

Signed-off-by: rfyiamcool <rfyiamcool@163.com>
Copy link
Contributor

@bxcodec bxcodec left a comment

Choose a reason for hiding this comment

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

LGTM

@bxcodec bxcodec merged commit 5423ebf into go-faker:main Sep 21, 2023
2 checks passed
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