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

Add "replace" function and documentation #140

Merged
merged 1 commit into from May 18, 2017

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented May 18, 2017

This pull request adds a replace function which takes three arguments - an original string, the substring to replace, and the string with which to replace it. This is of particular use when generating node names from IP addresses where the node name may not contain "." characters.

The strings.Replace function in the Go standard library seems like it would be better named strings.ReplaceN, so we wrap it to replace all occurences of the string. If necessary, a gomplate function named replaceN could be introduced calling strings.Replace directly.

@hairyhenderson
Copy link
Owner

Hi @jen20 - thanks for the PR! This is awesome - I was thinking just the other day that I needed a replace function 😉

Looks like the CI build is failing - should be pretty simple to fix, it seems you just need to run go fmt and fix up the imports - see https://circleci.com/gh/hairyhenderson/gomplate/452 for details.

stringfunc.go Outdated
// StringFunc - string manipulation function wrappers
type StringFunc struct {}

func (t StringFunc) Replace(s, old, new string) string {
Copy link
Owner

Choose a reason for hiding this comment

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

If necessary, a gomplate function named replaceN could be introduced calling strings.Replace directly.

To avoid the future name-shuffling (because I think exposing the full strings.Replace is inevitable) I think I'd prefer to name this function replaceAll.

And as an aside, can you lower-case the r just so that this isn't public API right away? I feel a bit boxed in with all the exported funcs I've written to date, and I'd like to stop that and make new ones non-exported for now.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you move the s arg to the end of the signature? Like this:

func (t StringFunc) replaceAll(old, new, s string) string {

Then it can be used to also support pipelines:

Like this:
{{ replaceAll "a" "o" "banana" }}
or this:
{{ "banana" | replaceAll "a" "o" }}

A number of other gomplate built-in funcs support this usage, so it'd fit in nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think all of this has been addressed with the latest push - I'll wait to watch CI complete now I'm not in a plane ;-)

"github.com/stretchr/testify/assert"
)

func TestReplace(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

Should be renamed to TestReplaceAll

Copy link
Owner

@hairyhenderson hairyhenderson left a comment

Choose a reason for hiding this comment

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

One minor nitpick, otherwise LGTM! Thanks so much for doing this work.

This commit adds a `replaceAll` function which takes three arguments - an
original string, the substring to replace, and the string with which to
replace it. This is of particular use when generating node names from IP
addresses where the node name may not contain "." characters.
@jen20
Copy link
Contributor Author

jen20 commented May 18, 2017

The last one should now be addressed. Do you have a particular schedule you are following for release builds?

@hairyhenderson hairyhenderson merged commit 8218e45 into hairyhenderson:master May 18, 2017
@hairyhenderson
Copy link
Owner

Thanks again, @jen20! I don't have a particular schedule - I should be able to get 1.7.0 out within the next day or so.

@jen20 jen20 deleted the replace branch May 18, 2017 23:03
@jen20
Copy link
Contributor Author

jen20 commented May 18, 2017

Great - thanks for merging!

@hairyhenderson
Copy link
Owner

I should be able to get 1.7.0 out within the next day or so.

well, better late than never 😬... 1.7.0 is out now!

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

Successfully merging this pull request may close these issues.

None yet

2 participants