Introduce documentation-support module
#428
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| getSimpleClassName(sourceFile.getName()))); | ||
| } | ||
|
|
||
| // XXX: `JavaFileObject` will most likely be added as parameter to help identify other `DocType`s. |
There was a problem hiding this comment.
This is just to point out and pick up in the next iteration.
| package tech.picnic.errorprone.plugin; | ||
|
|
||
| enum DocumentationType { | ||
| BUG_PATTERN("bugpattern", new BugPatternExtractor()); |
There was a problem hiding this comment.
Here we can add the others as well :).
rickie
force-pushed
the
rossendrijver/PSM-1716
branch
2 times, most recently
from
fa4c310 to
38235c1
Compare
rickie
added
chore
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
4 similar comments
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
2 similar comments
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
nathankooij
force-pushed
the
rossendrijver/PSM-1716
branch
from
5db7116 to
51ac167
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
@rickie fixed the Windows tests. Will review the code ~today. |
documentation-generator/pom.xml
Outdated
| <version>0.6.1-SNAPSHOT</version> | ||
| </parent> | ||
|
|
||
| <artifactId>documentation-generator</artifactId> |
There was a problem hiding this comment.
A completely different alternative: documentation-support.
There was a problem hiding this comment.
Any thoughts on this one @rickie? :)
There was a problem hiding this comment.
The suggestion is pretty good. The current one is IMO more inline with "refaster-compiler" and "refaster-runner". The former is most comparable to what we are doing in this module I would say.
The "-support" suffix to me indicates that others can easily use this for their own use cases / websites. (This may actually hold for some classes of this module 🤔, which is a good thing though and would make -support the better option.) OTOH how we use it and set it up is quite specific to our use case, which would make me lean towards the current name.
Not a very clear line of reasoning TBH, in essence both are fine by me. Was mostly not responding yet maybe others would've some input on this :).
There was a problem hiding this comment.
Discussed with @Stephan202 and settled on a name. It's the alternative you brought up @nathankooij ! It captures the goal nicely and is similar to some other modules.
documentation-generator/pom.xml
Outdated
| <artifactId>documentation-generator</artifactId> | ||
|
|
||
| <name>Picnic :: Error Prone Support :: Documentation Generation</name> | ||
| <description>Extracts data to generate the documentation</description> |
There was a problem hiding this comment.
| <description>Extracts data to generate the documentation</description> | |
| <description>Extracts data to generate the documentation.</description> |
| /** | ||
| * A {@link DocumentationExtractor} that describes how to extract data from a {@code BugChecker}. | ||
| */ | ||
| public final class BugPatternExtractor implements DocumentationExtractor<BugPatternData> { |
There was a problem hiding this comment.
Does this need to be public?
There was a problem hiding this comment.
Don't think so actually :).
| import tech.picnic.errorprone.plugin.models.BugPatternData; | ||
|
|
||
| /** | ||
| * A {@link DocumentationExtractor} that describes how to extract data from a {@code BugChecker}. |
There was a problem hiding this comment.
We use BugPattern and BugChecker interchangeably. Let's make sure we are consistent in what we mean.
There was a problem hiding this comment.
Hmm yes, we should. However I kind of want to propose to defer that decision as we would also want to solve that for the whole project in one go.
|
|
||
| @Override | ||
| public BugPatternData extractData(ClassTree tree, TaskEvent taskEvent) { | ||
| ClassSymbol symbol = ASTHelpers.getSymbol(tree); |
There was a problem hiding this comment.
Should we include a checkArgument here to enforce that this is indeed a BugPattern, e.g. annotation must be present.
| .resolve("docs" + File.separator + "bugpattern-CompilerBasedTestInput.json") | ||
| .toFile() | ||
| .exists()) | ||
| .isFalse(); |
There was a problem hiding this comment.
AssertJ also provides shorthands for Files and Paths, so let's use that. :)
| "public final class MinimalTestChecker extends BugChecker {}"); | ||
|
|
||
| CharSequence expectedJson = | ||
| FileObjects.forResource(getClass(), "bugpattern_example_minimal_testdata.json") |
There was a problem hiding this comment.
Think it's nicer to use the Resources API for this.
| assertThat( | ||
| Files.readString( | ||
| outputPath.resolve( | ||
| "docs" + File.separator + "bugpattern-CompilerBasedTestInput.json"))) |
There was a problem hiding this comment.
| "docs" + File.separator + "bugpattern-CompilerBasedTestInput.json"))) | |
| Paths.of("docs", "bugpattern-CompilerBasedTestInput.json"))) |
or simply chain Path#resolve.
| import org.junit.jupiter.api.condition.EnabledOnOs; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
|
|
||
| final class DocumentationGeneratorBugPatternTest extends DocumentationGeneratorCompilerBasedTest { |
There was a problem hiding this comment.
Something that came to mind after reading this test: it might be nicer to separate the writing and identification concerns into separate classes.
|
|
||
| abstract class DocumentationGeneratorCompilerBasedTest { | ||
| public void compile(String outputDirectory, String... lines) { | ||
| compile(outputDirectory, FileObjects.forSourceLines("CompilerBasedTestInput.java", lines)); |
There was a problem hiding this comment.
Should we extract this to a constant since we refer to it in other places?
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
rickie
force-pushed
the
rossendrijver/PSM-1716
branch
from
7720e00 to
688dc83
Compare
|
Rebased and applied almost all feedback that was left. Did some minor changes I discussed with @Stephan202 as well :). Thanks for the extensive review @nathankooij 🚀:heart:. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
I wasn't expecting this TBH. Not sure if I have time to dive into the Windows issue again 😬. |
Stephan202
force-pushed
the
rossendrijver/PSM-1716
branch
from
302274e to
485c85c
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
There was a problem hiding this comment.
Added one more commit. Suggested commit message:
Introduce `documentation-support` module (#428)
This new module provides the initial version of a framework for the extraction
of data from bug checkers and Refaster rules, to be used as input for website
generation.
| public static void compile(Path outputDirectory, String fileName, String... lines) { | ||
| performCompilationForFile( | ||
| outputDirectory.toAbsolutePath().toString(), FileObjects.forSourceLines(fileName, lines)); | ||
| } | ||
|
|
||
| public static void compile(String outputDirectory, String fileName, String... lines) { | ||
| performCompilationForFile(outputDirectory, FileObjects.forSourceLines(fileName, lines)); | ||
| } |
There was a problem hiding this comment.
The first method can delegate to the second.
| @Test | ||
| void emptyDirectoryWhenNotStartingKindAnalyze(@TempDir Path directory) { | ||
| Path outputPath = directory.resolve("pkg"); | ||
| JavacTaskCompilation.compile(outputPath, "A.java", "package pkg;"); | ||
|
|
||
| assertThat(directory).isEmptyDirectory(); | ||
| } | ||
|
|
||
| @Test | ||
| void noClassNoOutput(@TempDir Path directory) { | ||
| Path outputPath = directory.resolve("pkg"); | ||
| JavacTaskCompilation.compile(outputPath, "A.java", "package pkg;"); | ||
|
|
||
| assertThat(directory).isEmptyDirectory(); | ||
| } |
| javacFileManager, | ||
| null, | ||
| ImmutableList.of( | ||
| "-Xplugin:DocumentationGenerator -XoutputDirectory=" + outputDirectory), |
There was a problem hiding this comment.
This class' name is more generic than warranted by the implementation. Will propose something.
| * purposes from processed files. | ||
| */ | ||
| @AutoService(Plugin.class) | ||
| public final class DocumentationGenerator implements Plugin { |
There was a problem hiding this comment.
K, let's defer this; will add an XXX comment.
|
|
||
| // XXX: Generalize and move this class so that it can also be used by `refaster-compiler`. | ||
| // XXX: Add support for this class to the `ErrorProneTestHelperSourceFormat` check. | ||
| public final class JavacTaskCompilation { |
|
|
||
| /** A {@link BugChecker} that validates the {@link BugPatternExtractor}. */ | ||
| @BugPattern(summary = "Validates `BugPatternExtractor` extraction", severity = ERROR) | ||
| public static final class TestChecker extends BugChecker implements ClassTreeMatcher { |
There was a problem hiding this comment.
Let's keep inner classes at the bottom.
|
|
||
| @Test | ||
| void emptyDirectoryWhenNotStartingKindAnalyze(@TempDir Path directory) { | ||
| Path outputPath = directory.resolve("pkg"); |
There was a problem hiding this comment.
Here and below: the .resolve("pkg") isn't relevant to the test.
| final class DocumentationGeneratorTaskListenerTest { | ||
| @EnabledOnOs(WINDOWS) | ||
| @Test | ||
| void readOnlyFileSystemWindows(@TempDir Path directory) throws IOException { |
There was a problem hiding this comment.
| void readOnlyFileSystemWindows(@TempDir Path directory) throws IOException { | |
| void readOnlyFileSystemWindows(@TempDir Path outputDirectory) throws IOException { |
Etc.
| @Test | ||
| void skipTaskListenerStartedCreatesNoDirectories(@TempDir Path directory) { | ||
| Path outputPath = directory.resolve("pkg").toAbsolutePath(); | ||
| JavaFileObject javaFileObject = | ||
| FileObjects.forSourceLines( | ||
| "A.java", | ||
| "package pkg;", | ||
| "", | ||
| "import com.google.errorprone.bugpatterns.BugChecker;", | ||
| "", | ||
| "public final class A extends BugChecker {}"); | ||
| JavaCompiler compiler = JavacTool.create(); | ||
| JavacTaskImpl task = | ||
| (JavacTaskImpl) | ||
| compiler.getTask( | ||
| null, | ||
| null, | ||
| null, | ||
| ImmutableList.of("-Xplugin:DocumentationGenerator -XoutputDirectory=" + outputPath), | ||
| ImmutableList.of(), | ||
| ImmutableList.of(javaFileObject)); | ||
|
|
||
| task.parse(); | ||
| TaskEvent taskEvent = new TaskEvent(Kind.ANALYZE, javaFileObject); | ||
|
|
||
| for (TaskListener tl : task.getTaskListeners()) { | ||
| tl.finished(taskEvent); | ||
| } | ||
|
|
||
| assertThat(directory).isEmptyDirectory(); | ||
| } |
There was a problem hiding this comment.
This test doesn't seem too add much; suggest we drop it.
There was a problem hiding this comment.
That's okay, but dropping it will give a Pitest warning :).
There was a problem hiding this comment.
I'm open to suggestions that cover the relevant branch in a less contrived manner 🙃
| .hasMessage("'%s' must be of the form '%s=<value>'", pathArg, OUTPUT_DIRECTORY_FLAG); | ||
| } | ||
|
|
||
| @EnabledOnOs(WINDOWS) |
There was a problem hiding this comment.
We can instead use a path that's invalid on all supported OS'es.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Cool stuff btw 🚀! Will check out the changes in more detail later :). |
|
Very nice changes @Stephan202, will merge this one 😄! |
This is the PR that introduces this module. After polishing it and merging it, we can add more extractors for the following things:
While reviewing, keep in mind that we'll add the above mentioned changes soon and this PR is the setup for those extractors as well.
Having some doubts about the naming "documentation generation" vs. "documentation generator" in some places. I'm curious what you guys think :).