Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java I've updated the title of the bug. Siba can update the PR title. You're right that it's not easy to discover such APIs. We put it in `FileUtils` hoping people who does not want to write their own method will search from there first. As for the API itself, we've imagined something like FileUtils.patch(inputFile) .with(1, 10, List.of(lines), List.of(newLines)) .with(100, 100, linesAsAString, newLinesAsAString) .writeTo(outputFile) but the current form is the simplest case and could be kept even if we have a verbose one. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java Please change the issue/pr title to indicate it is a new API in the **test** library. The problem with single purpose APIs with not enough forethought is that they are not discoverable in the library and fall into disuse or are not appropriate for someone else to find and use. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java Currently it's just for one test, but it's the exact kind of method that should go into the test library, i.e. a general purpose file manipulation utility that can be useful by everyone. `patch` overwrites the input file (by default) too. We can always enhance it to support `-o`. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java If it is just for one/few tests, it would fit better in the directory with the test or in the test itself. This doesn't seem like enough of a general purpose function to be in the test library. Overwriting the input file seems like a bad choice, it will be a trap for someone. It prevents the use of the function in a case where the output is written to a different file. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java Or we can provide an example in the method spec. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java I can provide additional Test to Test this Test library. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java We have a test that dynamically downloads a source file, patches it, compiles it, and then runs it. We don't want to include a static copy of the file inside the test. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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: > > Update FileUtils.java Can you elaborate on the use case, what tests would it be used in? The hardcoded `from` and `to` seem very rigid and require knowledge of the exact format. That might lead to very fragile tests. Is this equivalent to looking for a substring aka `String.contains(xxx)` of the input file to replace (with line ending normalization)? - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 16:06:22 GMT, Daniel Fuchs wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update FileUtils.java
>
> test/lib/jdk/test/lib/util/FileUtils.java line 402:
>
>> 400: if (!removed.equals(froms)) {
>> 401: throw new IOException("Removed not the same");
>> 402: }
>
> That's a bit strange. I would suggest to return the removed lines instead, or
> to pass a `Consumer` (or even better, a `Predicate` ?) that
> will accept the removed lines. You could continue to remove if the predicate
> returns true and throw if it returns false. It would also enable you to tell
> exactly which line failed the check.
I was just thinking about providing the removed lines and the added lines at
the same time into the method (just like what a patch file looks like). The
exception here can probably be enhanced to compare the content of `removed`
with `from`. Two blocks of code (call and callback) would be needed if a
consumer or predicate is used, and I don't feel it's worth doing. Here I've
already trimmed the lines to make sure whitespaces do not matter.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 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:
>
> Update FileUtils.java
test/lib/jdk/test/lib/util/FileUtils.java line 402:
> 400: if (!removed.equals(froms)) {
> 401: throw new IOException("Removed not the same");
> 402: }
That's a bit strange. I would suggest to return the removed lines instead, or
to pass a `Consumer` (or even better, a `Predicate` ?) that
will accept the removed lines. You could continue to remove if the predicate
returns true and throw if it returns false. It would also enable you to tell
exactly which line failed the check.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:30:57 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: > > Update FileUtils.java API doc added along with some changes in the code. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
> 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: Update FileUtils.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/0ec01e0b..ef5dc31a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=01-02 Stats: 24 lines in 1 file changed: 15 ins; 0 del; 9 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
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v2]
On Fri, 22 Apr 2022 12:36:15 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:
>
> Update FileUtils.java
test/lib/jdk/test/lib/util/FileUtils.java line 381:
> 379:
> 380: public static void patch(Path path, int fromLine, int toLine, String
> to) throws IOException {
> 381: if(fromLine < 1 || toLine < 1) {
It would be good to add a proper API doc comment, especially regarding the
meaning of the parameters, and whether the line in question is
included/excluded. Also RuntimeException could be replaced with
IndexOutOfBoundsException.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v2]
> 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: Update FileUtils.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/39fa164a..0ec01e0b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
RFR: 8285452: Support new API to replace a file content using FileUtils.java
A new API to support replacing selective lines with desired content. - Commit messages: - 8285452: Support new API to replace a file content using FileUtils.java - Revert "8285452: Support new API to replace a file content in FileUtils.java" - 8285452: Support new API to replace a file content in FileUtils.java Changes: https://git.openjdk.java.net/jdk/pull/8360/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8360=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285452 Stats: 15 lines in 1 file changed: 15 ins; 0 del; 0 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
