-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-8274172: Convert JavadocTester to use NIO #5644
JDK-8274172: Convert JavadocTester to use NIO #5644
Conversation
👋 Welcome back jjg! A progress list of the required criteria for merging this PR into |
@jonathan-gibbons The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
test/langtools/jdk/javadoc/doclet/testRelativeLinks/TestRelativeLinks.java
Show resolved
Hide resolved
test/langtools/jdk/javadoc/doclet/testSearchScript/TestSearchScript.java
Show resolved
Hide resolved
test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java
Outdated
Show resolved
Hide resolved
test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java
Outdated
Show resolved
Hide resolved
test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java
Outdated
Show resolved
Hide resolved
Move `JavadocTester.copyDir` to `ToolBox`; use FileVisitor code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jon, thanks for modernizing JavadocTester! Before you read my suggestions, please consider this. I see a review as an opportunity to learn and improve, not just to mold a PR into a mainline commit.
In this particular case, I want to figure out the right way of copying a file tree. Why? Because I see opportunities of reusing this code in javadoc. For example, we have similar functionality in DocFilesHandlerImpl.copyDirectory. Why not spend extra time to figure out, if only a javadoc-internal, reference way of copying file trees?
Revert bad IDE suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed on the condition that you won't push it with
Files.createDirectories(toDir.toAbsolutePath().getParent())
as you suggested earlier.
@jonathan-gibbons This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 16 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 5b0c9cc.
Your commit was automatically rebased without conflicts. |
@jonathan-gibbons Pushed as commit 5b0c9cc. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review a moderately simple update to convert JavadocTester to just use NIO, instead of a mix of File and NIO.
The original code used java.io.File. At some point (JDK 9-ish) new code was added that used NIO, resulting in a mix. This change converts the old code to use NIO as well.
This is mostly internal, with two changes that affect tests.
protected
fieldoutputDir
is changed from aFile
to aPath
. Some tests useoutputDir
directly, typically to convert it to aPath
.copyDir
method had a strange spec. Partly, it used "target" to describe the directory being copied, but worse, it copied the entire source directory INTO the destination directory, as compared to copying the contents. The method was just used in a single test, so I've changed the spec of the method and the use in the test. This cleaned up a "TODO" as well, to useFiles.walkFileTree
for the copy.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5644/head:pull/5644
$ git checkout pull/5644
Update a local copy of the PR:
$ git checkout pull/5644
$ git pull https://git.openjdk.java.net/jdk pull/5644/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5644
View PR using the GUI difftool:
$ git pr show -t 5644
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5644.diff