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

refactor: Function capitalize word #460

Merged
merged 7 commits into from
Feb 24, 2023

Conversation

allensuvorov
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #488

Problem Summary

Team, thank you for the quick PR review.
By email I got xdlbdy's comment with implementation idea, (not sure how to find it on GitHub) so here is the screenshot:
image

Questions:

  1. For many reasons I like that implementation better. For one - it coverts to the runes from the get go. Can I use it instead of mine for this issue?
  2. And how about using !unicode.IsSpace as a criteria that we are inWord? If we use unicode.IsLetter(r) || unicode.IsNumber(r) the logic will treat other symbols as not inWord. For example a string let 'em go, will be capitalized to Let 'Em Go. I guess the question is, how do we want to capitalize cases like 'em and other cases where the word starts with a symbol that is neither a number nor a letter.

What is changed and how does it work?

  • improve readability.
  • convert to []rune from the get go.
  • handle empty string and single-symbol string cases in the main loop, not as special cases.
  • handle words with a leading non-letter or non-number, e.g. 'em
  • add a test case for 'em.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

@xdlbdy
Copy link
Contributor

xdlbdy commented Feb 24, 2023

pls resolve conflict

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #460 (184beab) into main (2a78cb5) will decrease coverage by 0.51%.
The diff coverage is 34.98%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #460      +/-   ##
==========================================
- Coverage   57.71%   57.20%   -0.51%     
==========================================
  Files         180      184       +4     
  Lines       14614    15005     +391     
==========================================
+ Hits         8434     8584     +150     
- Misses       5552     5791     +239     
- Partials      628      630       +2     
Impacted Files Coverage Δ
...ernal/controller/trigger/validation/subscripton.go 69.29% <ø> (+0.85%) ⬆️
internal/gateway/proxy/deadletter.go 0.00% <0.00%> (ø)
internal/gateway/proxy/direct.go 73.33% <0.00%> (-5.24%) ⬇️
internal/store/segment/server.go 14.10% <0.00%> (-0.03%) ⬇️
internal/trigger/client/gcloud_functions.go 0.00% <0.00%> (ø)
internal/trigger/client/grpc.go 0.00% <0.00%> (ø)
internal/trigger/client/http.go 0.00% <0.00%> (ø)
internal/trigger/client/interface.go 0.00% <0.00%> (-19.24%) ⬇️
internal/trigger/client/lambda.go 0.00% <0.00%> (ø)
internal/gateway/proxy/proxy.go 23.08% <6.52%> (-2.01%) ⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1da484d...184beab. Read the comment docs.

Copy link
Contributor

@xdlbdy xdlbdy left a comment

Choose a reason for hiding this comment

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

LGTM

@xdlbdy xdlbdy merged commit eee9250 into vanus-labs:main Feb 24, 2023
wenfengwang pushed a commit that referenced this pull request Mar 23, 2023
* feat: implement function capitalize_word

* test: add test TestCapitalizeWordAction

fix: in CapitalizeWord add strings.Join(capWords, " ") to return.

* fix: in CapitalizeWord use Fields instead of Split to takecare of dup spaces.

* fix: redo function to keep spaces and handle unicode, add tests.

* fix: update comment for NewCapitalizeWordAction

* refactor: better readability, cleaner code in CapitalizeWord. Add test case.
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.

2 participants