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 StringSlice type to sync #136

Merged
merged 2 commits into from
Sep 19, 2022
Merged

Conversation

peterklijn
Copy link
Collaborator

@peterklijn peterklijn commented Sep 15, 2022

Implements a simple version of issue #66

Comment on lines +425 to +428
for _, item := range strings.Split(val, ",") {
slice = append(slice, strings.TrimSpace(item))
}
s.Set(slice)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Do we want to trim spaces?
I'm not aware of any use cases, just thinking about it since trimming trailing and leading spaces seems more strict than not trimming them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason is that this will often be used for configuration, and a leading/trailing space can be hard to spot when something is failing.
The original stringMap was first code in API Gateway, and we actually had an issue during deployment because of a space, which is why this code has trims :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense 👍 .
I guess we can assume that in most cases we don't need leading and trailing spaces.

And as discussed, if we need more advanced options, such as supporting leading/trailing spaces and , we could introduce a more powerful mechanism (such as quotes and escaping).

@peterklijn peterklijn merged commit 08e0868 into beatlabs:master Sep 19, 2022
@peterklijn peterklijn deleted the add-string-slice branch September 19, 2022 10:20
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

3 participants