RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi Roger, The sentence has been changed and here is the updated one http://cr.openjdk.java.net/~rraghavan/8164781/webrev.04/ . The line length has also been fixed as suggested by Paul to adhere to ~80 chars. I have finalized the csr too. Regards Vivek -Original Message- From: Roger Riggs Sent: Thursday, April 05, 2018 6:35 PM To: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8164781: Pattern.asPredicate specification is incomplete Hi Vivek, Can we do something to make the first sentence less confusing? How about: * Creates a predicate that tests a given input string for a subsequence that matches this pattern. -or- * Creates a predicate that tests if this pattern is found in a given input string. Thanks, Roger On 4/5/18 6:34 AM, Vivek Theeyarath wrote: >>> Hi, >>> I have incorporated the changes as per the feedback and here is the >>> updated webrev . >>> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >>> >> +1 > Thanks Paul > >> I know it’s picky, but would you mind sticking closer to the existing >> line length in the source file (no need for another review) > Here is my attempt. Hope it is better now. > http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 > >> Did you run the jtreg test to verify it passes? I missed the problem >> initially, glad Stuart caught it, but i presume the test would of reported a >> failure? if not there is something wrong with the test itself that should be >> investigated. > > I missed it earlier. As you rightly pointed out, without Stuart's inputs the > test fails and it passes with it as expected. > >>> Here is the related csr >>> https://bugs.openjdk.java.net/browse/JDK-8200603 >>> >> Ok, i tweaked some of the information (after creating a CSR one often needs >> to edit it to fill in the gaps). >> Can you blockquote the markdown for the embedded patch since the formatting >> is all messed up? > I have restored the formatting. Hope it would suffice. > https://bugs.openjdk.java.net/browse/JDK-8200603 > > Regards > Vivek
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
> On Apr 5, 2018, at 3:34 AM, Vivek Theeyarath > wrote: > >>> >>> Hi, >>> I have incorporated the changes as per the feedback and here is the >>> updated webrev . >>> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >>> > >> +1 > Thanks Paul > >> I know it’s picky, but would you mind sticking closer to the existing line >> length in the source file >> (no need for another review) > Here is my attempt. Hope it is better now. > http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 > Nope, still not aligned to ~80 chars. >> Did you run the jtreg test to verify it passes? I missed the problem >> initially, glad Stuart caught it, but i presume the test would of reported a >> failure? if not there is something wrong with the test itself that should be >> investigated. > > > I missed it earlier. As you rightly pointed out, without Stuart's inputs the > test fails and it passes with it as expected. > >>> Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 >>> > >> Ok, i tweaked some of the information (after creating a CSR one often needs >> to edit it to fill in the gaps). > >> Can you blockquote the markdown for the embedded patch since the formatting >> is all messed up? > > I have restored the formatting. Hope it would suffice. > https://bugs.openjdk.java.net/browse/JDK-8200603 > Much better. Paul.
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi Vivek, Can we do something to make the first sentence less confusing? How about: * Creates a predicate that tests a given input string for a subsequence that matches this pattern. -or- * Creates a predicate that tests if this pattern is found in a given input string. Thanks, Roger On 4/5/18 6:34 AM, Vivek Theeyarath wrote: Hi, I have incorporated the changes as per the feedback and here is the updated webrev . http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 +1 Thanks Paul I know it’s picky, but would you mind sticking closer to the existing line length in the source file (no need for another review) Here is my attempt. Hope it is better now. http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 Did you run the jtreg test to verify it passes? I missed the problem initially, glad Stuart caught it, but i presume the test would of reported a failure? if not there is something wrong with the test itself that should be investigated. I missed it earlier. As you rightly pointed out, without Stuart's inputs the test fails and it passes with it as expected. Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 Ok, i tweaked some of the information (after creating a CSR one often needs to edit it to fill in the gaps). Can you blockquote the markdown for the embedded patch since the formatting is all messed up? I have restored the formatting. Hope it would suffice. https://bugs.openjdk.java.net/browse/JDK-8200603 Regards Vivek
RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
>> >> Hi, >> I have incorporated the changes as per the feedback and here is the >> updated webrev . >> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >> >+1 Thanks Paul >I know it’s picky, but would you mind sticking closer to the existing line >length in the source file >(no need for another review) Here is my attempt. Hope it is better now. http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 >Did you run the jtreg test to verify it passes? I missed the problem >initially, glad Stuart caught it, but i presume the test would of reported a >failure? if not there is something wrong with the test itself that should be >investigated. I missed it earlier. As you rightly pointed out, without Stuart's inputs the test fails and it passes with it as expected. >> Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 >> >Ok, i tweaked some of the information (after creating a CSR one often needs to >edit it to fill in the gaps). >Can you blockquote the markdown for the embedded patch since the formatting is >all messed up? I have restored the formatting. Hope it would suffice. https://bugs.openjdk.java.net/browse/JDK-8200603 Regards Vivek
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
> On Apr 3, 2018, at 11:54 PM, Vivek Theeyarath > wrote: > > Hi, > I have incorporated the changes as per the feedback and here is the > updated webrev . > http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . > Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 > +1 I know it’s picky, but would you mind sticking closer to the existing line length in the source file (no need for another review) Did you run the jtreg test to verify it passes? I missed the problem initially, glad Stuart caught it, but i presume the test would of reported a failure? if not there is something wrong with the test itself that should be investigated. > Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 > Ok, i tweaked some of the information (after creating a CSR one often needs to edit it to fill in the gaps). Can you blockquote the markdown for the embedded patch since the formatting is all messed up? Thanks, Paul. > I will try to address Uwe's point with a fix separately. > > Regards > Vivek > -Original Message- > From: Stuart Marks > Sent: Wednesday, April 04, 2018 6:13 AM > To: Vivek Theeyarath > Cc: Paul Sandoz ; Core-Libs-Dev > > Subject: Re: RFR: 8164781: Pattern.asPredicate specification is incomplete > > Hi Vivek, > > Thanks for taking on this task. > > In case it wasn't clear from Paul's mail, what I think you should do is > continue with this fix as a doc-only (and test-only) change, and not modify > the behavior of this method. Doing that would be an incompatible change. > > Uwe's point is a reasonable one, which is that you can't tell from the method > name "asPredicate" whether it uses find() or match() semantics. Oh well, I > think we just have to live with this, and document it clearly. > > Adding a method to create a Predicate that has match() semantics would be a > fine task to consider separately. > > Also, in RegExTest.java, > > 4686 if (p.test("word1234")) { > 4687 failCount++; > 4688 } > > I think the logic should be negated, as the predicate should properly find > the pattern in this string. > > Thanks, > > s'marks > > On 4/2/18 10:56 AM, Paul Sandoz wrote: >> Hi, >> >> Looks good, expect for: >> >> 5823 * @return The predicate which can be used for finding on a string >> >> “finding on a… ” is a little awkward to parse . I recommend to either change >> it back, since the first sentence of the method doc says what it means by >> matches, or being a little more verbose: >> >> The predicate which can be used for finding a match on a >> subsequence of a string >> >> You will need a CSR to document the clarification in specification behavior. >> >> — >> >> To Uwe’s point, we could have chosen a more descriptive method name, e.g. >> asFinding/Predicate, leaving logical space for say any future >> asMatching/Predicate if we chose to add it. >> >> Paul. >> >> >>> On Apr 1, 2018, at 1:11 AM, Vivek Theeyarath >>> wrote: >>> >>> Hi all, >>> >>> Please review. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >>> >>> Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/ >>> >>> >>> >>> Regards >>> >>> Vivek >>
RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi, I have incorporated the changes as per the feedback and here is the updated webrev . http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 I will try to address Uwe's point with a fix separately. Regards Vivek -Original Message- From: Stuart Marks Sent: Wednesday, April 04, 2018 6:13 AM To: Vivek Theeyarath Cc: Paul Sandoz ; Core-Libs-Dev Subject: Re: RFR: 8164781: Pattern.asPredicate specification is incomplete Hi Vivek, Thanks for taking on this task. In case it wasn't clear from Paul's mail, what I think you should do is continue with this fix as a doc-only (and test-only) change, and not modify the behavior of this method. Doing that would be an incompatible change. Uwe's point is a reasonable one, which is that you can't tell from the method name "asPredicate" whether it uses find() or match() semantics. Oh well, I think we just have to live with this, and document it clearly. Adding a method to create a Predicate that has match() semantics would be a fine task to consider separately. Also, in RegExTest.java, 4686 if (p.test("word1234")) { 4687 failCount++; 4688 } I think the logic should be negated, as the predicate should properly find the pattern in this string. Thanks, s'marks On 4/2/18 10:56 AM, Paul Sandoz wrote: > Hi, > > Looks good, expect for: > > 5823 * @return The predicate which can be used for finding on a string > > “finding on a… ” is a little awkward to parse . I recommend to either change > it back, since the first sentence of the method doc says what it means by > matches, or being a little more verbose: > >The predicate which can be used for finding a match on a > subsequence of a string > > You will need a CSR to document the clarification in specification behavior. > > — > > To Uwe’s point, we could have chosen a more descriptive method name, e.g. > asFinding/Predicate, leaving logical space for say any future > asMatching/Predicate if we chose to add it. > > Paul. > > >> On Apr 1, 2018, at 1:11 AM, Vivek Theeyarath >> wrote: >> >> Hi all, >> >>Please review. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >> >> Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/ >> >> >> >> Regards >> >> Vivek >
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
On 4/3/18 5:43 PM, Stuart Marks wrote: Adding a method to create a Predicate that has match() semantics would be a fine task to consider separately. I notice I had already filed a bug for this: https://bugs.openjdk.java.net/browse/JDK-8184692 Feel free to pick it up. s'marks
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi Vivek, Thanks for taking on this task. In case it wasn't clear from Paul's mail, what I think you should do is continue with this fix as a doc-only (and test-only) change, and not modify the behavior of this method. Doing that would be an incompatible change. Uwe's point is a reasonable one, which is that you can't tell from the method name "asPredicate" whether it uses find() or match() semantics. Oh well, I think we just have to live with this, and document it clearly. Adding a method to create a Predicate that has match() semantics would be a fine task to consider separately. Also, in RegExTest.java, 4686 if (p.test("word1234")) { 4687 failCount++; 4688 } I think the logic should be negated, as the predicate should properly find the pattern in this string. Thanks, s'marks On 4/2/18 10:56 AM, Paul Sandoz wrote: Hi, Looks good, expect for: 5823 * @return The predicate which can be used for finding on a string “finding on a… ” is a little awkward to parse . I recommend to either change it back, since the first sentence of the method doc says what it means by matches, or being a little more verbose: The predicate which can be used for finding a match on a subsequence of a string You will need a CSR to document the clarification in specification behavior. — To Uwe’s point, we could have chosen a more descriptive method name, e.g. asFinding/Predicate, leaving logical space for say any future asMatching/Predicate if we chose to add it. Paul. On Apr 1, 2018, at 1:11 AM, Vivek Theeyarath wrote: Hi all, Please review. Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/ Regards Vivek
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
On Sun, Apr 1, 2018 at 3:56 AM, Uwe Schindler wrote: > > In general, I'd prefer to have a predicate that uses matches() instead of > find(). In my code I have never used find() because in most cases you want > to match the whole string anyways. Of course you can use ^ and $ but I > don't like that leniency. If I write code, I prefer to be explicit in the > code what should done. > I have the opposite experience. find()'s behavior is the default with grep and perl regexes, so the behavior of matches() is the one I trip over.
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi, Looks good, expect for: 5823 * @return The predicate which can be used for finding on a string “finding on a… ” is a little awkward to parse . I recommend to either change it back, since the first sentence of the method doc says what it means by matches, or being a little more verbose: The predicate which can be used for finding a match on a subsequence of a string You will need a CSR to document the clarification in specification behavior. — To Uwe’s point, we could have chosen a more descriptive method name, e.g. asFinding/Predicate, leaving logical space for say any future asMatching/Predicate if we chose to add it. Paul. > On Apr 1, 2018, at 1:11 AM, Vivek Theeyarath > wrote: > > Hi all, > > Please review. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 > > Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/ > > > > Regards > > Vivek
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi, In general, I'd prefer to have a predicate that uses matches() instead of find(). In my code I have never used find() because in most cases you want to match the whole string anyways. Of course you can use ^ and $ but I don't like that leniency. If I write code, I prefer to be explicit in the code what should done. So I would prefer to add another asPredicate method that uses matches. I was stumbling on that several times. Uwe Am April 1, 2018 8:11:47 AM UTC schrieb Vivek Theeyarath : >Hi all, > > Please review. > >Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 > >Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/ > > > >Regards > >Vivek
RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi all, Please review. Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/ Regards Vivek