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

Continuation of add StringUtils.substringBeforeLast(String, int) #1325

Closed
wants to merge 1 commit into from

Conversation

Serwios
Copy link

@Serwios Serwios commented Dec 2, 2024

Hi, it's continuation of #967 , basically just added tests to @mmariotti PR, please review @arturobernalg @garydgregory

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible but is a best-practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that commits might be squashed by a maintainer on merge.

@Serwios
Copy link
Author

Serwios commented Dec 7, 2024

Hi, request to review, thanks @garydgregory

@garydgregory
Copy link
Member

  • I don't think this is needed or that we should cover every combination of before/after/char/int/any/not and so on.
  • don't delete existing tests
  • whenever I see int for a character, I think about code points, UTF-16, and supplementary characters which the tests here don't cover.

@Serwios
Copy link
Author

Serwios commented Dec 7, 2024

I didn't delete any tests, just added new one, old test testSubstringBetween_StringString is below my test

If you think it's not needed then I'll close this pr

@Serwios Serwios closed this Dec 7, 2024
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