On Mon, 29 Aug 2022 13:49:52 GMT, Raffaello Giulietti <d...@openjdk.org> wrote:

>> Add support for named groups to java.util.regex.MatchResult
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8065554: MatchResult should provide values of named-capturing groups

test/jdk/java/util/regex/NamedGroupsTests.java line 51:

> 49:         testMatchResultStartEndGroup2();
> 50:         testMatchResultStartEndGroup3();
> 51:     }

The three numbered tests here are a little hard to follow. Looks like these 
tests are

1. test existing group names, with no match
2. test existing group names, with a successful match
3. test nonexistent group names, with a successful match

On test names, sometimes people provide extremely verbose test names such as 
`testThatExistingGroupNameWithMatchReturnsNegativeOrNull` which I think is 
overkill, but having a name that's somewhat descriptive would be helpful.

It looks like a case is missing, which is a test for a nonexistent group name 
on a MatchResult that doesn't have a successful match. I'm not sure which is 
checked first; I think the implementation would throw IAE, because of the 
nonexistent name, regardless of whether or not the MatchResult has a match.

However, I don't think we've specified this, and in fact I don't think we want 
to. In general though, if multiple error conditions can arise in the same 
operation, the general style is not to constrain implementations to check for 
things in a particular order. Thus either IAE or ISE would be acceptable. 
Perhaps a test should be added for that. (Hm, might want to take another look 
at the specs regarding this issue.)

-------------

PR: https://git.openjdk.org/jdk/pull/10000

Reply via email to