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

Create an extension library for working with strings #316

Merged
merged 10 commits into from
Mar 9, 2020

Conversation

TristonianJones
Copy link
Collaborator

This code is based on FR #306 which requested support for built-in substrings. While substrings are not part of the core CEL spec, such functions are a common ask and the following is an implementation of such functions as extensions which can be layered onto the core CEL spec.

Closes #306

@TristonianJones
Copy link
Collaborator Author

@wlynch FYI

ext/strings.go Outdated Show resolved Hide resolved
ext/strings.go Outdated Show resolved Hide resolved
ext/strings.go Outdated Show resolved Hide resolved
ext/strings.go Outdated Show resolved Hide resolved
ext/strings.go Outdated Show resolved Hide resolved
ext/strings_test.go Show resolved Hide resolved
ext/strings_test.go Outdated Show resolved Hide resolved
ext/strings_test.go Outdated Show resolved Hide resolved
ext/strings_test.go Outdated Show resolved Hide resolved
ext/strings_test.go Show resolved Hide resolved
@bitsofinfo
Copy link

please add a replace function for simple string replacements

@TristonianJones
Copy link
Collaborator Author

@bitsofinfo I'm happy to report replace is in this PR! :)

@bitsofinfo
Copy link

solid! awesome, so for Tekton to utilize it I assume they as well will have to cut a new release updating the dependency to whatever version this will be in.

@TristonianJones
Copy link
Collaborator Author

PTAL

ext/strings.go Outdated
// Replace
//
// Produces a new string based on the target, which replaces the occurrences of a search string
// with a replacement string if present. Accepts an optional argument specifying a limit on the
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify that a negative argument means unlimited replacements.

ext/strings.go Outdated Show resolved Hide resolved
ext/strings_test.go Outdated Show resolved Hide resolved
ext/strings_test.go Show resolved Hide resolved
Copy link
Collaborator Author

@TristonianJones TristonianJones left a comment

Choose a reason for hiding this comment

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

PTAL

ext/strings.go Outdated Show resolved Hide resolved
ext/strings.go Outdated
// Replace
//
// Produces a new string based on the target, which replaces the occurrences of a search string
// with a replacement string if present. Accepts an optional argument specifying a limit on the
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

ext/strings.go Show resolved Hide resolved
ext/strings_test.go Show resolved Hide resolved
@TristonianJones
Copy link
Collaborator Author

PTAL

@TristonianJones TristonianJones merged commit 9ea4bb6 into google:master Mar 9, 2020
@TristonianJones TristonianJones deleted the ext-strings branch March 9, 2020 18:06
@bigkevmcd
Copy link

Would it be possible to get a doc page with the list of functions / examples?

@TristonianJones
Copy link
Collaborator Author

TristonianJones commented Mar 10, 2020 via email

@bigkevmcd
Copy link

@TristonianJones I was hoping for something I could link to from tektoncd/triggers to help people who want to use the functions, they're not necessarily Go developers.

@bigkevmcd
Copy link

The functions look great, and I'm happy to open a PR with a markdown with a list of functions examples in cel-go rather than in tektoncd/triggers which might help more folks.

@TristonianJones
Copy link
Collaborator Author

TristonianJones commented Mar 10, 2020

@bigkevmcd Sure, are you just looking for a markdown file for the strings, or would you like all the CEL functions documented that way. In the fullness of time, I'm sure both are desired, but if you need one sooner than the other, that helps me prioritize.

@bigkevmcd
Copy link

@TristonianJones Right now, the new string functions would help a lot, as soon as there's a release of cel-go with them, I have a PR ready to go.

@TristonianJones
Copy link
Collaborator Author

@bigkevmcd I'll work with @JimLarson to fix a minor issue I found in the library and copy in a markdown file. To use the new functions you'll have to use ext.Strings() in the CEL environment setup if you haven't started already.

I let @wlynch and @bobcatfish know offline that I think the truncate function won't handle unicode properly, but these extension functions will, so it might be worth updating to substring for these cases. I also inquired about static validation of CEL expressions rather than runtime-based validation. If there's some forum I should join to discuss I'd be happy to do so there.

@bitsofinfo
Copy link

@TristonianJones and @bigkevmcd thanks for brining this together, will be super handy in tekton

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.

FR: Built-in substring functions
4 participants