Feature/#610 Configure plugin locations - #611
kaklakariada wants to merge 20 commits into
Conversation
|
61 files. Dude, you are killing me! 🤣 I will do my very best to stay focused on the review. |
| loads each configured plugin through a single separate ClassLoader, matching the semantics of | ||
| a plugin directory under `$HOME/.oft/plugins/<plugin-name>/`. A plugin's JAR files may contain |
There was a problem hiding this comment.
"matching the semantics of" is hard to understand. Please rephrase.
| { | ||
| final Path jar = createJar("plugin.jar"); | ||
| final Oft oft = Oft.builder().addPlugin("my-plugin", jar).build(); | ||
| assertThat(oft, instanceOf(OftRunner.class)); |
There was a problem hiding this comment.
Completeness: This lacks a check whether adding the plugin did anything.
There was a problem hiding this comment.
Correctness. I know you did not change the file name. But when I look at this, it looks more like an integration test than a unit test.
|
|
||
| private static Path defaultPluginsDirectory() | ||
| { | ||
| return Path.of(System.getProperty("user.home")).resolve(".oft").resolve("plugins"); |
There was a problem hiding this comment.
Praise: very good. I was struggling with whether the directory should be .openfasttrace or .oft. But since the binary is oft, the name you chose is probably better.
There was a problem hiding this comment.
The name was already defined before, this just appeared during refactoring.
| /** | ||
| * Builder for creating {@link Oft} instances with custom configuration. | ||
| * <p> | ||
| * Use this builder to configure additional plugins that OFT should load at runtime in addition to the plugins |
There was a problem hiding this comment.
Maintainability: while this sentence is technically true, it might not survive changes. A more generic mention of custom configuration would be more robust.
There was a problem hiding this comment.
Uniformity: Have you considered inlining the Builder as Oft.Builder? I think that is how we did it in other builders and we should keep that structure.
| assertThat(plugin.getJars(), contains(jar1, jar2)); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Maintainability: Could you combine the similar bad-weather cases as parameterized test, please?
|
This branch has not been deployed
2 participants
Closes #610