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

[pkg/stanza] Extract trim func from split package #26536

Merged

Conversation

djaglowski
Copy link
Member

@djaglowski djaglowski commented Sep 8, 2023

Follows #26241

Previously, split funcs were responsible for applying trim funcs. This PR increases composability by applying trim funcs as a wrapper around split funcs.

One nuance that was surfaced here is that the newline split func was not handling the case where a line starts with a newline. When this happens, we need to tell the scanner to advance, but we still want to return a "" token, rather than nil. This is covered by existing tests, but previously it was "fixed" by the trim func which would return an empty slice when the token was nil. Now, the newline split func will explicitly handle this case, while the trim func will return the original value if it is nil or empty.

@github-actions github-actions bot requested a review from atoulme September 8, 2023 12:01
@djaglowski djaglowski force-pushed the pkg-stanza-extract-trim-split branch 8 times, most recently from a136bd7 to bf7aa90 Compare September 12, 2023 17:25
@djaglowski djaglowski force-pushed the pkg-stanza-extract-trim-split branch from bf7aa90 to 9e5f20e Compare September 12, 2023 18:00
@djaglowski djaglowski marked this pull request as ready for review September 12, 2023 18:40
@djaglowski djaglowski requested a review from a team September 12, 2023 18:40
@djaglowski djaglowski merged commit 82d0db2 into open-telemetry:main Sep 13, 2023
90 of 92 checks passed
@djaglowski djaglowski deleted the pkg-stanza-extract-trim-split branch September 13, 2023 18:57
@github-actions github-actions bot added this to the next release milestone Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants