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 string.format #617

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Conversation

DangerOnTheRanger
Copy link
Contributor

@DangerOnTheRanger DangerOnTheRanger commented Dec 19, 2022

This PR adds a format receiver function to CEL strings. The syntax is similar to the printf implementations found in various languages. For example, "%s and a number: %d".format(["a string", 42]) evaluates to "a string and a number: 42".

Copy link
Collaborator

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

Not a full review pass, but noticed a few things.

common/types/string_test.go Outdated Show resolved Hide resolved
common/types/string_test.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
common/types/string_test.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
@TristonianJones
Copy link
Collaborator

@DangerOnTheRanger Please move this functionality to ext/strings.go since string formatting would be an extension function that's optionally enabled rather than a part of the CEL standard library.

@TristonianJones
Copy link
Collaborator

@DangerOnTheRanger I have some questions based on what I've seen. Can you tell me the intended behavior for the following calls:

// error based on list literal size or simply a reuse of the first argument in the string?
"{0} < {0}".format([1, 2]) 

// How are brackets escaped?
"{{0}: {1}}".format([key, value])

It sounds like you and Joe have adequately answered whether the digits are Ascii-only, and I'm fine with that as well.

@DangerOnTheRanger
Copy link
Contributor Author

Kermit Alexander II I have some questions based on what I've seen. Can you tell me the intended behavior for the following calls:

// error based on list literal size or simply a reuse of the first argument in the string?
"{0} < {0}".format([1, 2]) 

// How are brackets escaped?
"{{0}: {1}}".format([key, value])

It sounds like you and Joe have adequately answered whether the digits are Ascii-only, and I'm fine with that as well.

For the first one, as currently written the implementation would accept this (the first argument would be reused) but after offline discussion with @jpbetz I think this should result in a compile-time error if the formatting string is a constant.

For the second, that would result in an error ({{ is treated as an escape sequence so the first } is unmatched), which feels reasonable to me. Admittedly the "correct form" of the formatting string ({{{0}: {1}}}) does look very verbose, but for use cases where the user wants to use the same characters that are used for formatting, (like { and } in this case) I'm not sure there's a way around that.

@DangerOnTheRanger DangerOnTheRanger mentioned this pull request Jan 27, 2023
3 tasks
common/types/string.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
common/types/string_test.go Outdated Show resolved Hide resolved
common/types/string_test.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
common/types/string_test.go Outdated Show resolved Hide resolved
common/types/string.go Outdated Show resolved Hide resolved
@DangerOnTheRanger DangerOnTheRanger changed the title [WIP] Add string.format Add string.format Feb 1, 2023
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 Outdated Show resolved Hide resolved
ext/strings.go Show resolved Hide resolved
cel/env.go Outdated Show resolved Hide resolved
ext/strings.go Show resolved Hide resolved
ext/strings.go Outdated Show resolved Hide resolved
ext/strings.go Outdated Show resolved Hide resolved
ext/strings_test.go Outdated Show resolved Hide resolved
ext/strings_test.go Outdated Show resolved Hide resolved
@jpbetz
Copy link
Collaborator

jpbetz commented Feb 14, 2023

/gcbrun

LGTM

@DangerOnTheRanger
Copy link
Contributor Author

(force push to fix merge conflict)

cel/decls.go Outdated Show resolved Hide resolved
ext/strings.go Outdated Show resolved Hide resolved
ext/strings_test.go Outdated Show resolved Hide resolved
ext/strings.go Show resolved Hide resolved
@TristonianJones
Copy link
Collaborator

/gcbrun

@TristonianJones
Copy link
Collaborator

/gcbrun

@TristonianJones
Copy link
Collaborator

/gcbrun

@cici37
Copy link
Collaborator

cici37 commented Feb 15, 2023

@jpbetz Would you mind checking if the request changes addressed? Looks like the change request is blocking the merging process on this one. Thank you :)

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.

5 participants