Yes, I noted that text blocks are not that great with string concatenation.
I agree with all you say, as I said, these are all questions of taste and it’s easy to get lost in them. I also think you spent a good amount of time tweaking the parameters already, and the result looks very good. So +1 for the patch as it is. Hannes > Am 05.05.2020 um 18:07 schrieb Jonathan Gibbons <jonathan.gibb...@oracle.com>: > > Hannes, > > Thanks for the feedback. > > It is easy enough to change the threshhold for wrapping very long lines. > I'll look at the other cases you mention, but the more we get into style > issues like "convert all or convert none", the harder it will be to come up > with rules for the converter. At some point we'll have to declare victory, > push the automated conversion, and then fix up other cases manually, either > in a single separate pass, or individually when we next edit the respective > files. > > The converter program has a length threshhold; in the first iteration, there > were cases where short strings were being converted and looked more clumsy > than the original. This was particularly true in some cases with interposed > values, like > "start of message " + value + " rest of message". > > -- Jon > > > On 5/5/20 8:55 AM, Hannes Wallnoefer wrote: >> Hi Jon, >> >> I find that using text blocks increase readability greatly, and the patch >> looks mostly very good to me. >> >> There are a few cases where I am not quite sure about the criteria used to >> decide whether or not to use text blocks. Nothing too bad but I think these >> are worth discussing: >> >> TestConstructors.java line 77: >> These are all short strings with a few escapes, it seems strange that one is >> converted to a text block just because it is a few characters longer. I >> think these strings should all be converted, or none of them. But I must >> admit it’s tough to decide where to draw the line. >> >> TestClassCrossReferences.java line starting at line 57: >> I like the wrapping of very long lines into actual „blocks“, but I think the >> threshold of 240 characters is too large in relation to the 80 characters >> pieces we chop them into. A line of 200 characters looks a bit like it was >> forgotten next to a wrapped block. IMO 120 or 160 characters would be a >> better threshold, i.e. a bit longer than the ideal length but still ok. >> I think my point is: chopped lines are quite easy to recognise as one big >> line, so no need to avoid them so long. >> >> TestHtmlVersion.java line 566: >> TestIndexInPackageFiles.java line 60: >> Why are these strings not converted? They are rather short, but they consist >> of 3 lines each and I think they woud benefit from text block representation. >> >> TestSearch.java line 326: >> These strings of Unicode escapes don’t seem to benefit from text block >> conversion, except for the \n at the end. Is that the reason they were >> included, or was it because of the unicode escapes? >> >> Hannes >> >> >>> Am 01.05.2020 um 23:13 schrieb Jonathan Gibbons >>> <jonathan.gibb...@oracle.com>: >>> >>> Please review a big, but conceptually simple, change to convert JavaDoc >>> tests to use text blocks where reasonable. >>> >>> This is an "automated" change, albeit using a custom new utility program >>> written for this specific purpose. There are no manual edits to any test >>> files. There should be no functional change in any test file. >>> >>> The conversion program scans source files looking for candidates to convert >>> into text blocks. Candidates are long strings containing '\n' or '"' >>> characters. >>> >>> Some tests contain very long strings, that previously were manually wrapped >>> at arbitrary points in the string. The converter program wraps these very >>> long strings (> 240 characters) every 80 characters. The fixed length of >>> each "chunk" and trailing '\' helps provide a visual distinctive block of >>> text for the long line. Note, this algorithm assumes there is no risk of >>> breaking low-level Unicode sequences within the strings in the test files. >>> >>> -- Jon >>> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8242532 >>> Webrev: http://cr.openjdk.java.net/~jjg/8242532/webrev.01/ >>> >>>
