Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
On Mon, 2 May 2022 13:13:02 GMT, Weijun Wang wrote: >> `lines.remove()` and `lines.subList()` will throw the correct exception. >> Since you asked, we can add it. > > Now that we call `subList` at the beginning, I think there's no need to > explicitly perform a check. Hello Weijun, the change to this part of the code in context of index checks, looks fine to me now. Thank you for considering it. - 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 [v5]
On Fri, 29 Apr 2022 15:51:12 GMT, Weijun Wang wrote:
>> test/lib/jdk/test/lib/util/FileUtils.java line 389:
>>
>>> 387: * @throws IOException
>>> 388: */
>>> 389: public static void patch(Path path, int fromLine, int toLine,
>>> String from, String to) throws IOException {
>>
>> Should this method check whether the `fromLine` and `toLine` are valid
>> values? Things like, negative value or 0 or `toLine` being less than
>> `fromLine`. I'm not familiar with the expectations of test library code -
>> maybe those checks aren't necessary since this is a test util?
>
> `lines.remove()` and `lines.subList()` will throw the correct exception.
> Since you asked, we can add it.
Now that we call `subList` at the beginning, I think there's no need to
explicitly perform a check.
-
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 [v5]
On Fri, 29 Apr 2022 12:02:59 GMT, Jaikiran Pai wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> updated to add space character in begining of multiline string
>
> test/lib/jdk/test/lib/util/FileUtils.java line 389:
>
>> 387: * @throws IOException
>> 388: */
>> 389: public static void patch(Path path, int fromLine, int toLine,
>> String from, String to) throws IOException {
>
> Should this method check whether the `fromLine` and `toLine` are valid
> values? Things like, negative value or 0 or `toLine` being less than
> `fromLine`. I'm not familiar with the expectations of test library code -
> maybe those checks aren't necessary since this is a test util?
`lines.remove()` and `lines.subList()` will throw the correct exception. Since
you asked, we can add it.
-
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 [v5]
On Fri, 29 Apr 2022 10:46:31 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:
>
> updated to add space character in begining of multiline string
test/lib/jdk/test/lib/util/FileUtils.java line 383:
> 381: * Patches a part of a file.
> 382: * @param path of file
> 383: * @param fromLine the first line to patch. This is the number you
> see in an editor, 1-based.
Perhaps this should mention whether the `fromLine` is inclusive, like it's
noted for the `toLine`?
test/lib/jdk/test/lib/util/FileUtils.java line 389:
> 387: * @throws IOException
> 388: */
> 389: public static void patch(Path path, int fromLine, int toLine, String
> from, String to) throws IOException {
Should this method check whether the `fromLine` and `toLine` are valid values?
Things like, negative value or 0 or `toLine` being less than `fromLine`. I'm
not familiar with the expectations of test library code - maybe those checks
aren't necessary since this is a test util?
-
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 [v5]
On Fri, 29 Apr 2022 10:46:58 GMT, Sibabrata Sahoo wrote: >> Sibabrata Sahoo has updated the pull request incrementally with one >> additional commit since the last revision: >> >> updated to add space character in begining of multiline string > > test/jdk/java/nio/file/Files/FileUtilsTest.java line 51: > >> 49: c"""; >> 50: // 1st line has a space character >> 51: String sabc = " " + System.lineSeparator() + abc; > > It's strange that jcheck fails, if there is space character in beginning of > line in a multiline string. So i have to follow this way add a space > character in the beginning of multiline string. You can use \s instead of space. Then you will have no complaints. - 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 [v5]
On Fri, 29 Apr 2022 10:46:31 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: > > updated to add space character in begining of multiline string test/jdk/java/nio/file/Files/FileUtilsTest.java line 51: > 49: c"""; > 50: // 1st line has a space character > 51: String sabc = " " + System.lineSeparator() + abc; It's strange that jcheck fails, if there is space character in beginning of line in a multiline string. So i have to follow this way add a space character in the beginning of multiline string. - 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 [v5]
> 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: updated to add space character in begining of multiline string - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/14125936..e18cd8cc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=03-04 Stats: 5 lines in 1 file changed: 0 ins; 4 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
