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

feat(transform): Support string joining arrays #5148

Merged
merged 1 commit into from Jan 24, 2024

Conversation

MisterMX
Copy link
Contributor

@MisterMX MisterMX commented Dec 19, 2023

Description of your changes

Add a new string tranform Join that wraps around strings.Join and converts any given array into a single string separated by a user defined string.

Docs PR: crossplane/docs#662

I have:

Need help with this checklist? See the cheat sheet.

@MisterMX MisterMX requested review from negz and a team as code owners December 19, 2023 14:58
@MisterMX MisterMX requested a review from phisco December 19, 2023 14:58
@MisterMX MisterMX force-pushed the feat/transform-string-join branch 2 times, most recently from 1e340d5 to e3ca469 Compare December 19, 2023 17:17
Comment on lines 503 to 506
case v1.StringTransformTypeJoin:
if t.String.Join == nil {
return errors.Errorf("string transform join type is required for join transform")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

here the fromType should be checked, given that this transform will only work with arrays as input, you should change this to:

Suggested change
case v1.StringTransformTypeJoin:
if t.String.Join == nil {
return errors.Errorf("string transform join type is required for join transform")
}
case v1.StringTransformTypeJoin:
if fromType != v1.TransformIOTypeArray {
return errors.Errorf("string transform join type can only be used with arrays as input, got %s", fromType)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be this function's job to validate the correctness of the API, just that the types match. We check that for StringTransformTypeConvert above only because we need to further switch on it to know the correct type. That's why I was suggesting replacing it altogether instead of adding it. But that doesn't change much, it's just a nit. Let me know if you want to address that or we can proceed further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 removed the changes

Add a new string tranform `Join` that wraps around `strings.Join` and
converts any given array into a single string separated by a user
defined string.

Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com>
@MisterMX MisterMX force-pushed the feat/transform-string-join branch 2 times, most recently from 9367b8f to 8d6af61 Compare January 24, 2024 11:56
Copy link
Contributor

@phisco phisco left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@phisco phisco merged commit 6220cc4 into crossplane:master Jan 24, 2024
15 of 17 checks passed
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.

None yet

2 participants