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 crstrings library #2

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Aug 30, 2024

This library contains some general convenience functions related to
strings and string slices.


This change is Reviewable

@RaduBerinde
Copy link
Member Author

Let me know if there are other utilities that you wish you had in the standard strings library. I plan to use some of these in Pebble soon.

@RaduBerinde RaduBerinde force-pushed the crstrings branch 2 times, most recently from 478b47b to f49c5dd Compare August 30, 2024 15:50
Copy link

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @RaduBerinde)


crstrings/utils.go line 73 at r1 (raw file):

//
//	fmt.Sprintf("%s%s", a, crstrings.PrependIfNotEmpty(", ", b))
func PrependIfNotEmpty(prefix, s string) string {

I understand what this is trying to do, but this sample usage did not help me understand the value.
Was this meant to help with formatting loops in prepending a separator on iterations after the first iteration?

Similar question for AppendIfNotEmpty.

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @sumeerbhola)


crstrings/utils.go line 73 at r1 (raw file):

Previously, sumeerbhola wrote…

I understand what this is trying to do, but this sample usage did not help me understand the value.
Was this meant to help with formatting loops in prepending a separator on iterations after the first iteration?

Similar question for AppendIfNotEmpty.

It's mostly for printf statements. E.g. Sprintf("%s%s %s%s", x, PrependIfNotEmpty(", ", y), z, PrependIfNotEmptry(", ", u) That would normally take 5+ lines of code.

@RaduBerinde
Copy link
Member Author

crstrings/utils.go line 73 at r1 (raw file):

Previously, RaduBerinde wrote…

It's mostly for printf statements. E.g. Sprintf("%s%s %s%s", x, PrependIfNotEmpty(", ", y), z, PrependIfNotEmptry(", ", u) That would normally take 5+ lines of code.

Though maybe it would be cleaner to do Sprintf("%s%s%s", a, crstrings.If(b != "", ", "), b)

Copy link

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @RaduBerinde)


crstrings/utils.go line 73 at r1 (raw file):

Previously, RaduBerinde wrote…

Though maybe it would be cleaner to do Sprintf("%s%s%s", a, crstrings.If(b != "", ", "), b)

Thanks for the clarification. Either is fine by me.

This library contains some general convenience functions related to
strings and string slices.
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @sumeerbhola)


crstrings/utils.go line 73 at r1 (raw file):

Previously, sumeerbhola wrote…

Thanks for the clarification. Either is fine by me.

I replaced them with a single WithSep(a, separator, b) function.

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @sumeerbhola)

@RaduBerinde RaduBerinde merged commit 3474b32 into cockroachdb:main Sep 4, 2024
6 of 7 checks passed
@RaduBerinde RaduBerinde deleted the crstrings branch September 4, 2024 21:00
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.

2 participants