8241187: ToolBox::grep should allow for negative filtering #1934
Conversation
|
|
Webrevs
|
| * @param lines the strings to be filtered | ||
| * @return the strings not matching the regular expression | ||
| */ | ||
| public List<String> grepNotMatch(Pattern pattern, List<String> lines) { |
There was a problem hiding this comment.
Instead of new methods named grepNotMatch I suggest adding new overloads of grep that take an additional boolean invert parameter that is conceptually equivalent to the grep -v option. The existing grep methods can be updated to delegate to the new methods, passing false for the new parameter.
There was a problem hiding this comment.
@jonathan-gibbons Thank you for your suggestion. The code is updated now.
|
@lgxbslgx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@lgxbslgx This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! |
|
/open |
|
@lgxbslgx @HostUserDetails{id=13688759, username='lgxbslgx', fullName='null'} this pull request is now open |
|
Ping. Could I ask your help to review this patch? Thanks a lot. |
| * false: positive filtering, return the matched strings | ||
| * @return the strings matching(or not matching) the regular expression | ||
| */ | ||
| public List<String> grep(Pattern pattern, List<String> lines, boolean invert) { |
There was a problem hiding this comment.
what about?
public List<String> grep(Pattern pattern, List<String> lines, boolean invert) {
return lines.stream()
.filter(s -> invert ? !pattern.matcher(s).find() : pattern.matcher(s).find())
.collect(Collectors.toList());
}
There was a problem hiding this comment.
Putting the invert into method filter may cause a little performance influence. Is it similar to loop?
Because this is the test code, I adopt your suggestion.
|
@lgxbslgx This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 311 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vicente-romero-oracle, @jonathan-gibbons) but any other Committer may sponsor as well.
|
| * @param invert identify positive or negative filtering | ||
| * true: negative filtering, return the unmatched strings | ||
| * false: positive filtering, return the matched strings |
There was a problem hiding this comment.
This is not a particularly well-formed javadoc comment. Imagine the output if you were to run the file through javadoc.
Suggest:
Filters a list of strings according to the given regular expression,
returning either the strings that match or the strings that do not match.
@param regex the regular expression
@param lines the strings to be filtered
@param invert if true, return the lines that do not match; otherwise if false, return the lines that do match.
The invert parameter feels "inverted" leading to a "double negative".
Maybe it would be better to call the parameter "match" and invert the sense, so the doc comment reads:
Filters a list of strings according to the given regular expression,
returning either the strings that match or the strings that do not match.
@param regex the regular expression
@param lines the strings to be filtered
@param match if true, return the lines that match; otherwise if false, return the lines that do not match.
|
@vicente-romero-oracle @jonathan-gibbons Thanks for your review. I revised the code according to your suggestion. |
| return lines.stream() | ||
| .filter(s -> pattern.matcher(s).find()) | ||
| .filter(s -> match ? pattern.matcher(s).find() : !pattern.matcher(s).find()) |
There was a problem hiding this comment.
s -> pattern.matcher(s).find() ^ !match would avoid repeating the expression
There was a problem hiding this comment.
Or probably even simpler s -> pattern.matcher(s).find() == match
There was a problem hiding this comment.
The first form is too cryptic. The second form is OK.
|
/integrate |
|
/sponsor |
openjdk
bot
added
integrated
|
@jonathan-gibbons @lgxbslgx Since your change was applied there have been 312 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit ed32e02. |
Hi all,
This patch adds two methods in
ToolBoxto do the negative filtering. Although the labelnoreg-selfwas added, I write a test for this enhancement to verify the code. And the method namegrepNotMatchmay need to be improved. Any idea is appreciated.Thank you for taking the time to review.
Best Regards.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/1934/head:pull/1934$ git checkout pull/1934Update a local copy of the PR:
$ git checkout pull/1934$ git pull https://git.openjdk.java.net/jdk pull/1934/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1934View PR using the GUI difftool:
$ git pr show -t 1934Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/1934.diff