Add extractMatches() to RegExUtils by komatzu1337 · Pull Request #1382 · apache/commons-lang

komatzu1337 · 2025-05-15T01:05:53Z

Provides simple way to get all regex matches as list without manual Matcher handling.

garydgregory

I'm not sure how this one. This is a pretty narrow use case. Some people will want a List, others an array; some will want String input, others will say it should be a CharSequence. What should happen if the string is null? What should happen if the pattern is null? Should an empty list (or array) be returned? A null? Should the List be immutable? Can this even be reused with Lang?

Supports both String and CharSequence Provides both List<String> and String[] return Added regex-string variant All returned collections are unmodifiable Never returns null (empty collections for null) Improved documentation

komatzu1337 · 2025-05-15T04:06:33Z

TY for your feedback! I implemented the following improvements:

Now throws IllegalArgumentException with clear messages for null
Supports both String and CharSequence
Provides both List and String[] return
Added regex-string variant
All returned collections are unmodifiable
Never returns null (empty collections for null)
Improved documentation

I will add tests later if you find this class worthwhile

ppkarwasz

Hi @komatzu1337,

Thanks for the PR, could you add some unit tests please?

Note also that you enabled "Vigilant mode" on your GitHub account, but you didn't add the GPG key you are using to your GitHub account. For this reason all your commits appear as "Unverified". Can you fix that?

garydgregory · 2025-05-15T11:11:11Z

I'm still not convinced this belongs here.

ppkarwasz · 2025-05-15T11:58:34Z

I'm still not convinced this belongs here.

@komatzu1337, I appreciate the effort you've put into this contribution. I also have some reservations about the utility of findMatches, so please view my comments as suggestions for improvement rather than a final decision on acceptance.

Just as a reminder, per ASF guidelines, all maintainers need to reach consensus before any changes are merged.

komatzu1337 · 2025-05-15T12:23:40Z

I'm still not convinced this belongs here.

why?

komatzu1337 · 2025-05-15T12:27:45Z

Hi @komatzu1337,

Thanks for the PR, could you add some unit tests please?

hi
i will add unit tests later today

Note also that you enabled "Vigilant mode" on your GitHub account, but you didn't add the GPG key you are using to your GitHub account. For this reason all your commits appear as "Unverified". Can you fix that?

i'll figure it out, thanks

class MatchFinder was removed now returns empty List if pattern or text null {} added on if, so RegExUtils will pass checks

ppkarwasz · 2025-05-16T21:35:28Z

Regarding the utility of this method, with the help of Copilot, I found some projects that have a similar method that returns List<String>:

Google Re2j: https://github.com/google/re2j/blob/84237cbbd0fbd637c6eb6856717c1e248daae729/java/com/google/re2j/RE2.java#L807-L822
Around 800 Java projects have a method called findAllMatches and some of them could reuse a method in Commons Lang.

ppkarwasz · 2025-05-16T21:45:40Z

I'm still not convinced this belongs here.

why?

When adding a new method to Commons Lang, we need to consider the impact on the entire ecosystem. Even small additions increase the library's size for thousands of downstream projects. Given the wide range of potential utility methods, we prioritize those likely to benefit a significant portion of users—let's say, at least 1%.

While your proposed method may be useful in some contexts, it might not offer enough broad utility to justify inclusion in the core library.

tests added public List<String> findMatches(String text, Pattern pattern) is removed

…inder

ppkarwasz

@komatzu1337,

As noted earlier, we're still not convinced that even a single findAll method is broadly useful. Including three separate implementations feels like more than we can justify at this point. Could you please limit the pull request to the findAll(CharSequence, Pattern) method, and optionally a second variant that delegates to it?

ppkarwasz · 2025-05-17T20:30:49Z

+     * @param pattern the pattern to search for
+     * @return an unmodifiable list of found matches; returns an empty List if text or pattern is null
+     */
+    public List<String> findMatches(CharSequence text, Pattern pattern) {


Suggested change

public List<String> findMatches(CharSequence text, Pattern pattern) {

public static List<String> findAll(CharSequence text, Pattern pattern) {

This is an utility class, so all the methods must be static.

Nitpick: Naming is certainly on of the hardest tasks. Since all the methods in this class deal with matching and have names like replaceAll, removeAll, I would call this method findAll.

ppkarwasz · 2025-05-17T20:34:03Z

+     * @param regex the regular expression pattern
+     * @return an array of found matches; returns an empty array if text or regex is null
+     */
+    public String[] findMatchesAsArray(CharSequence text, String regex) {


Looking at the remaining methods of this class, each method with signature (CharSequence, Pattern) has a corresponding method with signature (String, String). Therefore I would define a method:

public static List<String> findAll(String text, String regex)

I don't think it is useful to have both a method returning List<String> and String[]. The return type should be coherent.

ppkarwasz · 2025-05-17T20:36:14Z

+     * @param pattern the pattern to search for
+     * @return a modifiable ArrayList of found matches; returns an empty List if text or pattern is null
+     */
+    public List<String> findMatchesModifiable(CharSequence text, Pattern pattern){


-1

We should not start multiplying methods.

ppkarwasz · 2025-05-17T20:36:54Z

+    @BeforeEach
+    void setUp() {
+        utils = new RegExUtils();
+    }


Utility classes are never instantiated.

ppkarwasz · 2025-05-17T20:37:29Z

+    }
+
+    @Test
+    public void testFindMultipleMatches() {


Can you use parameterized logging instead of multiple methods with the same logic?

garydgregory

There are so many possibilities of combining Pattern, Matcher, and MatchResult that I don't see why this specific use case needs to be here. For example, where would this be reused in Commons Lang or from other Commons libraries?

ppkarwasz · 2025-05-17T21:26:35Z

There are so many possibilities of combining Pattern, Matcher, and MatchResult that I don't see why this specific use case needs to be here. For example, where would this be reused in Commons Lang or from other Commons libraries?

Good point! @komatzu1337, could you share a few concrete examples where this method would be especially useful—either in real-world projects or potential usage within Commons libraries?

The current unit tests illustrate some interesting scenarios, but they can be addressed using existing utilities:

Splitting a sentence into words can be done with StringUtils.split.
To extract numbers from a sentence, you could split into words and then filter those that are numeric.

Understanding your intended use cases would help us evaluate whether this method brings enough added value and general applicability to warrant inclusion here.

komatzu1337 · 2025-05-18T16:52:12Z

@ppkarwasz @garydgregory Thank you for your patience and valuable feedback.After considering your feedback, i agree that this method doesn’t align well with Commons Lang’s scope. I apologize for the time spent reviewing this proposal - in hindsight, I should have more thoroughly vetted the use case before submitting it.

This experience has helped me better understand Commons Lang's design philosophy. I will be sure to:

Review the contribution guidelines more carefully next time
Get feedback in advance through discussion questions for future changes.

Add extractMatches method implementation

ffebbf4

garydgregory reviewed May 15, 2025

View reviewed changes

ppkarwasz requested changes May 15, 2025

View reviewed changes

Comment thread src/main/java/org/apache/commons/lang3/RegExUtils.java Outdated

Comment thread src/main/java/org/apache/commons/lang3/RegExUtils.java Outdated

Comment thread src/main/java/org/apache/commons/lang3/RegExUtils.java Outdated

ppkarwasz mentioned this pull request May 16, 2025

improvement/multiple-fixes1 komatzu1337/commons-lang#2

Closed

added tests

8b27f2e

class MatchFinder was removed now returns empty List if pattern or text null {} added on if, so RegExUtils will pass checks

komatzu1337 added 3 commits May 17, 2025 14:08

Method findMatchesModifiable added

6842207

tests added public List<String> findMatches(String text, Pattern pattern) is removed

Method findMatchesModifiable added

b6ed649

tests added public List<String> findMatches(String text, Pattern pattern) is removed

Merge remote-tracking branch 'origin/add-matchfinder' into add-matchf…

0a4629a

…inder

komatzu1337 requested a review from ppkarwasz May 17, 2025 18:19

ppkarwasz requested changes May 17, 2025

View reviewed changes

garydgregory reviewed May 17, 2025

View reviewed changes

komatzu1337 closed this May 18, 2025

	public List<String> findMatches(CharSequence text, Pattern pattern) {
	public static List<String> findAll(CharSequence text, Pattern pattern) {

Conversation

komatzu1337 commented May 15, 2025

Uh oh!

garydgregory left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

komatzu1337 commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppkarwasz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garydgregory commented May 15, 2025

Uh oh!

ppkarwasz commented May 15, 2025

Uh oh!

komatzu1337 commented May 15, 2025

Uh oh!

komatzu1337 commented May 15, 2025

Uh oh!

ppkarwasz commented May 16, 2025

Uh oh!

ppkarwasz commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppkarwasz left a comment

Choose a reason for hiding this comment

Uh oh!

ppkarwasz May 17, 2025

Choose a reason for hiding this comment

Uh oh!

ppkarwasz May 17, 2025

Choose a reason for hiding this comment

Uh oh!

ppkarwasz May 17, 2025

Choose a reason for hiding this comment

Uh oh!

ppkarwasz May 17, 2025

Choose a reason for hiding this comment

Uh oh!

ppkarwasz May 17, 2025

Choose a reason for hiding this comment

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

ppkarwasz commented May 17, 2025

Uh oh!

komatzu1337 commented May 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

garydgregory left a comment •

edited

Loading

komatzu1337 commented May 15, 2025 •

edited

Loading

ppkarwasz commented May 16, 2025 •

edited

Loading