Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 18:28:45 GMT, Weijun Wang wrote: >> Shouldn't the comparison be better implemented by the caller then, who will >> know whether spaces are important or not? That's why I had suggested taking >> a `Predicate` that could be called with each line removed, and the >> caller could interrupt the parsing by returning false when they detect a >> mismatch with what they expect. > > We can provide a more sophisticated `Function` replacer if we > want to let user to customize all the details. This time we still only want > them to be string literals. I agree we can keep the new lines inside, but > trimming on each line and the final block is still useful so caller does not > need to care about indentation and empty lines at both ends. OK - if you keep the internal new lines I have no objection. The API doc should however say that lines will be trimmed before comparing them. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 14:19:40 GMT, Daniel Fuchs wrote: >> The comparison is intentionally made lax so the caller does not need to >> provide the exact indentation or even new line characters. We think along >> with `fromLine` and `toLine` this is enough to make sure we are not >> modifying the wrong lines. > > Shouldn't the comparison be better implemented by the caller then, who will > know whether spaces are important or not? That's why I had suggested taking a > `Predicate` that could be called with each line removed, and the > caller could interrupt the parsing by returning false when they detect a > mismatch with what they expect. We can provide a more sophisticated `Function` replacer if we want to let user to customize all the details. This time we still only want them to be string literals. I agree we can keep the new lines inside, but trimming on each line and the final block is still useful so caller does not need to care about indentation and empty lines at both ends. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:29:35 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8285452: updated to use lines() test/jdk/java/nio/file/Files/FileUtilsTest.java line 111: > 109: test(abcList, 1, 3, "ab", xyz, "xyz"); > 110: } > 111: Any thought of using TestNG with a DataProvider? Seems more efficient - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 13:02:06 GMT, Weijun Wang wrote: >> Also calling trim() assumes that white spaces are not significant. This >> might not be the case in the general case (for instance - think of markdown >> files were leading spaces are significant). > > The comparison is intentionally made lax so the caller does not need to > provide the exact indentation or even new line characters. We think along > with `fromLine` and `toLine` this is enough to make sure we are not modifying > the wrong lines. Shouldn't the comparison be better implemented by the caller then, who will know whether spaces are important or not? That's why I had suggested taking a `Predicate` that could be called with each line removed, and the caller could interrupt the parsing by returning false when it detects a mismatch with what they expect. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:44:26 GMT, Daniel Fuchs wrote:
>> test/lib/jdk/test/lib/util/FileUtils.java line 394:
>>
>>> 392: var removed = "";
>>> 393: for (int i = fromLine; i <= toLine; i++) {
>>> 394: removed += lines.remove(fromLine - 1).trim();
>>
>> shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c"
>> will be the same as concatenating lines "a" and "bc".
>
> Also calling trim() assumes that white spaces are not significant. This might
> not be the case in the general case (for instance - think of markdown files
> were leading spaces are significant).
The comparison is intentionally made lax so the caller does not need to provide
the exact indentation or even new line characters. We think along with
`fromLine` and `toLine` this is enough to make sure we are not modifying the
wrong lines.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:35:59 GMT, Daniel Fuchs wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8285452: updated to use lines()
>
> test/lib/jdk/test/lib/util/FileUtils.java line 394:
>
>> 392: var removed = "";
>> 393: for (int i = fromLine; i <= toLine; i++) {
>> 394: removed += lines.remove(fromLine - 1).trim();
>
> shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c" will
> be the same as concatenating lines "a" and "bc".
Also calling trim() assumes that white spaces are not significant. This might
not be the case in the general case (for instance - think of markdown files
were leading spaces are significant).
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:29:35 GMT, Sibabrata Sahoo wrote:
>> A new API to support replacing selective lines with desired content.
>
> Sibabrata Sahoo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8285452: updated to use lines()
test/lib/jdk/test/lib/util/FileUtils.java line 394:
> 392: var removed = "";
> 393: for (int i = fromLine; i <= toLine; i++) {
> 394: removed += lines.remove(fromLine - 1).trim();
shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c" will
be the same as concatenating lines "a" and "bc".
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
> A new API to support replacing selective lines with desired content. Sibabrata Sahoo has updated the pull request incrementally with one additional commit since the last revision: 8285452: updated to use lines() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/da6a214a..0b7dc2f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=05-06 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8360.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8360/head:pull/8360 PR: https://git.openjdk.java.net/jdk/pull/8360
