-
Notifications
You must be signed in to change notification settings - Fork 13
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
Support Strings CEL library #245
Conversation
This comment has been minimized.
This comment has been minimized.
9ad5ba4
to
4ec156e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
grpc/federation/cel/strings.go
Outdated
}, | ||
), | ||
), | ||
// func Cut(s, sep string) (before, after string, found bool) |
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.
What is this comment for? 🤔 Do we miss the Cut function?
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.
Sorry for the lack of explanation in the comments. I left a comment for the functions I determined to be impossible to implement. Except for Cut
, the other comments are for functions that have arguments of types (like Go's func
type) that cannot be received by CEL. As for Cut
, I concluded it was impossible to implement because it needs to return two variables.
Is there any way to return two variables in this case?
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.
We talked on this point directly and decided to update the comments to clarify why we cannot implement those functions.
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.
Do we miss the Cut function?
Regarding "Cut," I found that it's feasible to implement, so I added the implementation.
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 left some questions but overall it looks good to me 👍
This comment has been minimized.
This comment has been minimized.
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.
LGTM 👍
Waiting for review from @utahta.
Code Metrics Report
Details | | main (b948f38) | #245 (5cec434) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 65.8% | 66.0% | +0.2% |
| Files | 74 | 75 | +1 |
| Lines | 12347 | 12418 | +71 |
+ | Covered | 8123 | 8192 | +69 |
+ | Code to Test Ratio | 1:0.3 | 1:0.4 | +0.0 |
| Code | 38141 | 38524 | +383 |
+ | Test | 13210 | 13878 | +668 |
- | Test Execution Time | 7m29s | 7m44s | +15s | Code coverage of files in pull request scope (100.0% → 97.8%)
Reported by octocov |
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.
LGTM!
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.
🙆♂️
Provide functionalities similar to Go's strings using the grpc.federation.strings library.
#159