Skip to content
This repository was archived by the owner on May 23, 2022. It is now read-only.

Conversation

@Wulfheart
Copy link
Collaborator

@Wulfheart Wulfheart commented Feb 20, 2021

This pull request is opened to show a progress of the implementation of some string helpers from Laravel.

This will address confetti-framework/confetti#102

@Wulfheart Wulfheart changed the title Strhelpers [WIP] Strhelpers Feb 20, 2021
@reindert-vetter reindert-vetter self-requested a review February 20, 2021 16:28

func TestAfter(t *testing.T) {

assert.Equal(t, "nah", After("hannah", "han"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I start each test with empty values as parameters. As an example:
assert.Equal(t, "", After("", ""))
assert.Equal(t, "", After("hannah", ""))
assert.Equal(t, "", After("", "hannah"))
(I'm not sure if the result is what it should be.)

Would you like to add that to this test and check it in the other tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea.

@reindert-vetter
Copy link
Contributor

reindert-vetter commented Feb 20, 2021

This looks good and is very useful! 🥇

@codecov
Copy link

codecov bot commented Mar 13, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@5feb8c2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main       #5   +/-   ##
=======================================
  Coverage        ?   77.52%           
=======================================
  Files           ?       15           
  Lines           ?      574           
  Branches        ?        0           
=======================================
  Hits            ?      445           
  Misses          ?      108           
  Partials        ?       21           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5feb8c2...01573e5. Read the comment docs.

@reindert-vetter
Copy link
Contributor

Great changes! Do you mind merging main into this branch? Then we get a better idea of whether code coverage has improved.

}

func ContainsAllInSlice(haystack string, needle []string) bool {
func ContainsAllFromSlice(haystack string, needle []string) bool {
Copy link
Contributor

@reindert-vetter reindert-vetter Mar 13, 2021

Choose a reason for hiding this comment

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

What do you think of the following?
ContainsBySlice()
ContainsAllBySlice()
But it doesn't really matter to me 🤷‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course. I will change this. I had just worked for some minutes on it and simply pushed the changes. More to come when I have the time to do so.

}

// Return the remainder of a string after the first occurrence of a given value.
func After(subject string, search string) string {
Copy link
Collaborator

@octoper octoper Apr 11, 2021

Choose a reason for hiding this comment

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

This can be reduced to

func After(subject string, search string) string {
	if len(search) == 0 {
		return input
	}
	results := strings.SplitN(subject, search, 2)
	return results[len(results)-1]
}

}

// Return the remainder of a string after the last occurrence of a given value.
func AfterLast(subject string, search string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code can be reduced to

func AfterLast(subject string, search string) string {
	if len(search) == 0 {
		return subject
	}
	position := strings.LastIndex(subject, search)

	if position == -1 {
		return subject
	}

	return subject[position+len(search):]
}

}

// Get the portion of a string before the first occurrence of a given value.
func Before(subject string, search string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code can be reduced to

func Before(subject string, search string) string {
	if len(search) == 0 {
		return subject
	}
	position := strings.Index(subject, search)

	if position == -1 {
		return subject
	}

	return subject[:position]
}

return result
}

func BeforeLast(subject string, search string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code can be reduced to

func BeforeLast(subject string, search string) string {
	if len(search) == 0 {
		return subject
	}
	position := strings.LastIndex(subject, search)

	if position == -1 {
		return subject
	}

	return subject[:position]
}

@reindert-vetter
Copy link
Contributor

@octoper feel free to continue working on this PR. I don't know how you can easily work on this with @Wulfheart. Perhaps can fork Wulfheart:strhelpers and build on it. I think it would be good if this PR is not open for too long. Maybe it would be better if few functions work, we already merge them.

@Wulfheart
Copy link
Collaborator Author

@octoper I can give you write access to repository if you want and we can continue this fork together.

@octoper
Copy link
Collaborator

octoper commented Apr 11, 2021

Yeah @Wulfheart if I would love to help!

@Wulfheart
Copy link
Collaborator Author

I hope the invitation has been sent. How can we avoid duplication? How would you like to solve it? How can we coordinate it?

@octoper
Copy link
Collaborator

octoper commented Apr 11, 2021

Do you have a Discord account or something else so we can talk there or Twitter maybe?

@reindert-vetter
Copy link
Contributor

reindert-vetter commented Apr 11, 2021

@octoper @Wulfheart I have given you write access. You should have received an invitation. I will create a branch for the above.

@reindert-vetter
Copy link
Contributor

reindert-vetter commented Apr 11, 2021

New pull-request:
#12

@Wulfheart and @octoper you can develop on branch #5-string-helpers (in confetti-framework/support repo)

@octoper
Copy link
Collaborator

octoper commented Apr 11, 2021

@reindert-vetter Thank you!

octoper added a commit to confetti-framework/framework that referenced this pull request May 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants