-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add abbreviations for title case #41
Conversation
@dnnrly I was unable to set up godog locally. So i created this PR so that the CI runs here. |
@dnnrly how is it supposed to work without the code?
|
0792bdf
to
3735e3b
Compare
I have merged a change to master that should fix the problems you were having with Godog. I'm not sure why the |
I did some more checking on a rebased version of your branch - I think it might be the length is set wrong. Try |
@dnnrly fixed it. Can you have a look today? |
cmd/title.go
Outdated
abbr := domain.AsPascal(abbreviator, args[0], optMax, optFrmFront, optRemoveStopwords) | ||
|
||
if len(abbr) > 0 { | ||
s := string(abbr[0]) | ||
for i := 1; i < len(abbr); i++ { | ||
if string(abbr[i]) == strings.ToUpper(string(abbr[i])) { | ||
s += " " | ||
} | ||
s += string(abbr[i]) | ||
} | ||
|
||
fmt.Printf("%s", s) | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is decent implementation. I think to make it align with the the rest of the codebase it might be worth creating a new function ToTitle
in the domain
package that returns a string. That way we use unit tests to validate the behaviour and just have to print it out in this command function - this would be a great thing to see here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I just pushed a fix after refactoring and adding unit tests. Can you take a look now?
I think there's an error in the title command file: |
@dnnrly copy paste errors :) |
As is my tradition, you've earned a follow on Twitter. 😄 |
Description
Adding support for title casing.
Running example:
Fixes #39
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
make lint
)master