On Mon, 26 Aug 2024 09:22:59 GMT, Nizar Benalla <nbena...@openjdk.org> wrote:
>> Can I please get a review for this PR that adds 4 new html "Checkers" for >> the generated documentation. >> More details are in the JBS issues >> >> These tests were mostly inspired /converted from the existing >> [Doccheck](https://github.com/openjdk/doccheck). > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Fix path passed to the test Good start! Some suggestions inline test/langtools/jdk/javadoc/doccheck/DocCheck.java line 43: > 41: import java.util.concurrent.ExecutorService; > 42: import java.util.concurrent.Executors; > 43: import java.util.concurrent.Future; It is often common practice to put Java SE and JDK imports first. test/langtools/jdk/javadoc/doccheck/DocCheck.java line 56: > 54: @Before > 55: public void setUp() { > 56: Path root = Path.of(ROOT_PATH.getParent() + File.separator + > "docs" + File.separator + System.getProperty("doccheck.dir")); somewhat preferable to use `Path.resolve` instead of adding strings like this test/langtools/jdk/javadoc/doccheck/DocCheck.java line 71: > 69: BadCharacterChecker badChars = new BadCharacterChecker(); > 70: HtmlFileChecker docChecker = new HtmlFileChecker(new > DocTypeChecker()); > 71: HtmlFileChecker htmlChecker = new HtmlFileChecker(new > LinkChecker()); I suggest extracting these declarations into a method called something like `getCheckers()` and then having two alternative methods to call them, called homelike like `runSerially(List<Checker> checkers)` and `runWithExecutor(List<Checker> checkers)` ------------- PR Review: https://git.openjdk.org/jdk/pull/20711#pullrequestreview-2261580004 PR Review Comment: https://git.openjdk.org/jdk/pull/20711#discussion_r1731783076 PR Review Comment: https://git.openjdk.org/jdk/pull/20711#discussion_r1731784158 PR Review Comment: https://git.openjdk.org/jdk/pull/20711#discussion_r1731787106
