You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2025/06/16 06:31:13 UTC
[PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
paulk-asert opened a new pull request, #2254:
URL: https://github.com/apache/groovy/pull/2254
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on code in PR #2254:
URL: https://github.com/apache/groovy/pull/2254#discussion_r2155691786
##########
src/main/java/org/codehaus/groovy/runtime/StringGroovyMethods.java:
##########
@@ -1547,6 +1548,146 @@ private static List<String> getGroups(final Matcher matcher) {
return list;
}
+ // stateful matcher API is not very extensible; care should be taken if using the matcher directly as it will potentially be mutated
+ // this mostly provides an efficient list delegate but will less efficiently recalculates matches for named groups
+ static class GroupList implements List<String> {
+ private final Matcher matcher;
+ private final int count;
+ private final List<String> delegate;
+
+ public GroupList(Matcher matcher, int count) {
+ this.matcher = matcher;
+ this.count = count;
+ this.delegate = getGroups(matcher);
+ }
+
+ @Override
+ public int size() {
+ return delegate.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return delegate.isEmpty();
+ }
+
+ @Override
+ public boolean contains(Object o) {
+ return delegate.contains(o);
+ }
+
+ @Override
+ public Iterator<String> iterator() {
+ return delegate.iterator();
+ }
+
+ @Override
+ public Object[] toArray() {
+ return delegate.toArray();
+ }
+
+ @Override
+ public boolean add(String s) {
+ throw new UnsupportedOperationException("Can't add to a Matcher result");
+ }
+
+ @Override
+ public boolean remove(Object o) {
+ throw new UnsupportedOperationException("Can't remove a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(int index, Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public void clear() {
+ throw new UnsupportedOperationException("Can't clear a Matcher result");
+ }
+
+ @Override
+ public String get(int index) {
+ return delegate.get(index);
+ }
+
+ public String get(String name) {
+ matcher.reset();
+ for (int i = 0; i <= count; i++) {
Review Comment:
Understood
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun merged PR #2254:
URL: https://github.com/apache/groovy/pull/2254
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2254:
URL: https://github.com/apache/groovy/pull/2254#issuecomment-2985985015
Merged. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on code in PR #2254:
URL: https://github.com/apache/groovy/pull/2254#discussion_r2153612481
##########
src/main/java/org/codehaus/groovy/runtime/StringGroovyMethods.java:
##########
@@ -1547,6 +1548,146 @@ private static List<String> getGroups(final Matcher matcher) {
return list;
}
+ // stateful matcher API is not very extensible; care should be taken if using the matcher directly as it will potentially be mutated
+ // this mostly provides an efficient list delegate but will less efficiently recalculates matches for named groups
+ static class GroupList implements List<String> {
+ private final Matcher matcher;
+ private final int count;
+ private final List<String> delegate;
+
+ public GroupList(Matcher matcher, int count) {
+ this.matcher = matcher;
+ this.count = count;
+ this.delegate = getGroups(matcher);
+ }
+
+ @Override
+ public int size() {
+ return delegate.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return delegate.isEmpty();
+ }
+
+ @Override
+ public boolean contains(Object o) {
+ return delegate.contains(o);
+ }
+
+ @Override
+ public Iterator<String> iterator() {
+ return delegate.iterator();
+ }
+
+ @Override
+ public Object[] toArray() {
+ return delegate.toArray();
+ }
+
+ @Override
+ public boolean add(String s) {
+ throw new UnsupportedOperationException("Can't add to a Matcher result");
+ }
+
+ @Override
+ public boolean remove(Object o) {
+ throw new UnsupportedOperationException("Can't remove a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(int index, Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public void clear() {
+ throw new UnsupportedOperationException("Can't clear a Matcher result");
+ }
+
+ @Override
+ public String get(int index) {
+ return delegate.get(index);
+ }
+
+ public String get(String name) {
+ matcher.reset();
+ for (int i = 0; i <= count; i++) {
Review Comment:
@paulk-asert Any thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2254:
URL: https://github.com/apache/groovy/pull/2254#issuecomment-2981925028
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2254:
URL: https://github.com/apache/groovy/pull/2254#discussion_r2153222725
##########
src/main/java/org/codehaus/groovy/runtime/StringGroovyMethods.java:
##########
@@ -1547,6 +1548,146 @@ private static List<String> getGroups(final Matcher matcher) {
return list;
}
+ // stateful matcher API is not very extensible; care should be taken if using the matcher directly as it will potentially be mutated
+ // this mostly provides an efficient list delegate but will less efficiently recalculates matches for named groups
+ static class GroupList implements List<String> {
+ private final Matcher matcher;
+ private final int count;
+ private final List<String> delegate;
+
+ public GroupList(Matcher matcher, int count) {
+ this.matcher = matcher;
+ this.count = count;
+ this.delegate = getGroups(matcher);
+ }
+
+ @Override
+ public int size() {
+ return delegate.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return delegate.isEmpty();
+ }
+
+ @Override
+ public boolean contains(Object o) {
+ return delegate.contains(o);
+ }
+
+ @Override
+ public Iterator<String> iterator() {
+ return delegate.iterator();
+ }
+
+ @Override
+ public Object[] toArray() {
+ return delegate.toArray();
+ }
+
+ @Override
+ public boolean add(String s) {
+ throw new UnsupportedOperationException("Can't add to a Matcher result");
+ }
+
+ @Override
+ public boolean remove(Object o) {
+ throw new UnsupportedOperationException("Can't remove a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(int index, Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public void clear() {
+ throw new UnsupportedOperationException("Can't clear a Matcher result");
+ }
+
+ @Override
+ public String get(int index) {
+ return delegate.get(index);
+ }
+
+ public String get(String name) {
+ matcher.reset();
+ for (int i = 0; i <= count; i++) {
Review Comment:
The loop boundary in the get(String name) method uses 'i <= count', which may result in one extra call to matcher.find() if 'count' is intended to be zero-based. Please verify whether this should be 'i < count' to correctly locate the intended match.
```suggestion
for (int i = 0; i < count; i++) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2254:
URL: https://github.com/apache/groovy/pull/2254#issuecomment-2975592770
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2254?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `44.11765%` with `19 lines` in your changes missing coverage. Please review.
> Project coverage is 69.0475%. Comparing base [(`4090a96`)](https://app.codecov.io/gh/apache/groovy/commit/4090a96f4ec0f9859ec8139bc5ab77bb49f7a86f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`2089d28`)](https://app.codecov.io/gh/apache/groovy/commit/2089d2877ce8d051a1b6bd490ccb1bf3fde9c326?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2254?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...g/codehaus/groovy/runtime/StringGroovyMethods.java](https://app.codecov.io/gh/apache/groovy/pull/2254?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FStringGroovyMethods.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvU3RyaW5nR3Jvb3Z5TWV0aG9kcy5qYXZh) | 44.1176% | [19 Missing :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2254 +/- ##
==================================================
- Coverage 69.0517% 69.0475% -0.0042%
Complexity 29708 29708
==================================================
Files 1423 1423
Lines 114394 114427 +33
Branches 19831 19832 +1
==================================================
+ Hits 78991 79009 +18
- Misses 28769 28786 +17
+ Partials 6634 6632 -2
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2254?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...g/codehaus/groovy/runtime/StringGroovyMethods.java](https://app.codecov.io/gh/apache/groovy/pull/2254?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FStringGroovyMethods.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvU3RyaW5nR3Jvb3Z5TWV0aG9kcy5qYXZh) | `81.5327% <44.1176%> (-1.6915%)` | :arrow_down: |
... and [4 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2254/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2254:
URL: https://github.com/apache/groovy/pull/2254#discussion_r2153724057
##########
src/main/java/org/codehaus/groovy/runtime/StringGroovyMethods.java:
##########
@@ -1547,6 +1548,146 @@ private static List<String> getGroups(final Matcher matcher) {
return list;
}
+ // stateful matcher API is not very extensible; care should be taken if using the matcher directly as it will potentially be mutated
+ // this mostly provides an efficient list delegate but will less efficiently recalculates matches for named groups
+ static class GroupList implements List<String> {
+ private final Matcher matcher;
+ private final int count;
+ private final List<String> delegate;
+
+ public GroupList(Matcher matcher, int count) {
+ this.matcher = matcher;
+ this.count = count;
+ this.delegate = getGroups(matcher);
+ }
+
+ @Override
+ public int size() {
+ return delegate.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return delegate.isEmpty();
+ }
+
+ @Override
+ public boolean contains(Object o) {
+ return delegate.contains(o);
+ }
+
+ @Override
+ public Iterator<String> iterator() {
+ return delegate.iterator();
+ }
+
+ @Override
+ public Object[] toArray() {
+ return delegate.toArray();
+ }
+
+ @Override
+ public boolean add(String s) {
+ throw new UnsupportedOperationException("Can't add to a Matcher result");
+ }
+
+ @Override
+ public boolean remove(Object o) {
+ throw new UnsupportedOperationException("Can't remove a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(int index, Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public void clear() {
+ throw new UnsupportedOperationException("Can't clear a Matcher result");
+ }
+
+ @Override
+ public String get(int index) {
+ return delegate.get(index);
+ }
+
+ public String get(String name) {
+ matcher.reset();
+ for (int i = 0; i <= count; i++) {
Review Comment:
It all looks fine to me. You have to call find() at least once before accessing results and we have index 0 for that case. If we changed it from `count++` to `++count`, we'd need to follow the advice given.
```
def issueRegex = /(?<project>\w*)-(?<number>\d+)/
def m = 'GROOVY-11701 precedes GROOVY-11702 and follows GROOVY-11700' =~ issueRegex
assert m[0][0] == 'GROOVY-11701'
assert m[0][1] == 'GROOVY'
assert m[0][2] == '11701'
assert m[0]['project'] == 'GROOVY'
assert m[0].number == '11701'
assert m[1][0] == 'GROOVY-11702'
assert m[1][1] == 'GROOVY'
assert m[1][2] == '11702'
assert m[1]['project'] == 'GROOVY'
assert m[1].number == '11702'
assert m[2][0] == 'GROOVY-11700'
assert m[2][1] == 'GROOVY'
assert m[2][2] == '11700'
assert m[2]['project'] == 'GROOVY'
assert m[2].number == '11700'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11701: Improved named-capturing group support for Groovy matchers [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2254:
URL: https://github.com/apache/groovy/pull/2254#discussion_r2153724057
##########
src/main/java/org/codehaus/groovy/runtime/StringGroovyMethods.java:
##########
@@ -1547,6 +1548,146 @@ private static List<String> getGroups(final Matcher matcher) {
return list;
}
+ // stateful matcher API is not very extensible; care should be taken if using the matcher directly as it will potentially be mutated
+ // this mostly provides an efficient list delegate but will less efficiently recalculates matches for named groups
+ static class GroupList implements List<String> {
+ private final Matcher matcher;
+ private final int count;
+ private final List<String> delegate;
+
+ public GroupList(Matcher matcher, int count) {
+ this.matcher = matcher;
+ this.count = count;
+ this.delegate = getGroups(matcher);
+ }
+
+ @Override
+ public int size() {
+ return delegate.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return delegate.isEmpty();
+ }
+
+ @Override
+ public boolean contains(Object o) {
+ return delegate.contains(o);
+ }
+
+ @Override
+ public Iterator<String> iterator() {
+ return delegate.iterator();
+ }
+
+ @Override
+ public Object[] toArray() {
+ return delegate.toArray();
+ }
+
+ @Override
+ public boolean add(String s) {
+ throw new UnsupportedOperationException("Can't add to a Matcher result");
+ }
+
+ @Override
+ public boolean remove(Object o) {
+ throw new UnsupportedOperationException("Can't remove a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public boolean addAll(int index, Collection c) {
+ throw new UnsupportedOperationException("Can't addAll to a Matcher result");
+ }
+
+ @Override
+ public void clear() {
+ throw new UnsupportedOperationException("Can't clear a Matcher result");
+ }
+
+ @Override
+ public String get(int index) {
+ return delegate.get(index);
+ }
+
+ public String get(String name) {
+ matcher.reset();
+ for (int i = 0; i <= count; i++) {
Review Comment:
It all looks fine to me as is. You have to call find() at least once before accessing results and we have index 0 for that case. If we changed it from `count++` to `++count`, we'd need to follow the advice given.
```
def issueRegex = /(?<project>\w*)-(?<number>\d+)/
def m = 'GROOVY-11701 precedes GROOVY-11702 and follows GROOVY-11700' =~ issueRegex
assert m[0][0] == 'GROOVY-11701'
assert m[0][1] == 'GROOVY'
assert m[0][2] == '11701'
assert m[0]['project'] == 'GROOVY'
assert m[0].number == '11701'
assert m[1][0] == 'GROOVY-11702'
assert m[1][1] == 'GROOVY'
assert m[1][2] == '11702'
assert m[1]['project'] == 'GROOVY'
assert m[1].number == '11702'
assert m[2][0] == 'GROOVY-11700'
assert m[2][1] == 'GROOVY'
assert m[2][2] == '11700'
assert m[2]['project'] == 'GROOVY'
assert m[2].number == '11700'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org