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.

Sign up for GitHub

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

Jump to bottom

Refactoring to make PluginDescriptor more usable #180

Merged

decebals merged 6 commits into pf4j:master from josiahhaswell:PF4J-179

Nov 1, 2017

Contributor

josiahhaswell commented Nov 1, 2017

I need to be able to create a PluginDescriptor from YAML. This seems to me to make the system even more extensible.


          Refactoring to make PluginDescriptor more usable

ac5e705

coveralls commented Nov 1, 2017 •

edited

Loading

Coverage remained the same at 56.957% when pulling ac5e705 on josiahhaswell:PF4J-179 into a30c05b on decebals:master.


          Modifying mutators in DefaultPluginDescriptor to protected from packa…

3e54091

…ge-protected

coveralls commented Nov 1, 2017 •

edited

Loading

Coverage decreased (-0.4%) to 56.605% when pulling 3e54091 on josiahhaswell:PF4J-179 into a30c05b on decebals:master.

Member

decebals commented Nov 1, 2017

From #175 (comment) you can see that I planed some modifications related to PluginDescriptor.
I will take a look on your code and I will come with feedback.

decebals requested changes

View reviewed changes

pf4j/src/main/java/org/pf4j/DefaultPluginDescriptor.java Outdated

+               */
+              public class DefaultPluginDescriptor implements PluginDescriptor {
+              	private String pluginId;

Member

decebals Nov 1, 2017

Indentation

pf4j/src/main/java/org/pf4j/DefaultPluginDescriptor.java Outdated

+              public class DefaultPluginDescriptor implements PluginDescriptor {
+              	private String pluginId;
+              	private String pluginDescription;

Member

decebals Nov 1, 2017

Indentation

pf4j/src/main/java/org/pf4j/DefaultPluginDescriptor.java Outdated

+                   * @param provider
+                   * @param license
+                   */
+                  public DefaultPluginDescriptor(

Member

decebals Nov 1, 2017

I prefer a more compact format, maybe one line

pf4j/src/main/java/org/pf4j/DependencyResolver.java Outdated

@@ @@ -32,7 +32,7 @@ @@
                * they are not found dependencies, they are dependencies with wrong version).
                * This class is very useful for if-else scenarios.
                *
-               * Only some attributes (pluginId, dependencies and pluginVersion) from {@link PluginDescriptor} are used in
+               * Only some attributes (pluginId, dependencies and pluginVersion) from {@link DefaultPluginDescriptor} are used in

Member

decebals Nov 1, 2017

No need to change. I feel that it's from the refactoring action

pf4j/src/main/java/org/pf4j/ManifestPluginDescriptorFinder.java

                   }
                   protected PluginDescriptor createPluginDescriptor(Manifest manifest) {
-                      PluginDescriptor pluginDescriptor = createPluginDescriptorInstance();
+                      DefaultPluginDescriptor pluginDescriptor = createPluginDescriptorInstance();

Member

decebals Nov 1, 2017

No need to change. I feel that it's from the refactoring action

pf4j/src/test/java/org/pf4j/DefaultPluginFactoryTest.java Outdated

@@ @@ -38,7 +38,7 @@ @@
                    */
                   @Test
                   public void testCreate() {
-                      PluginDescriptor pluginDescriptor = mock(PluginDescriptor.class);
+                      PluginDescriptor pluginDescriptor = mock(DefaultPluginDescriptor.class);

Member

decebals Nov 1, 2017

No need to change. I feel that it's from the refactoring action

pf4j/src/test/java/org/pf4j/DefaultPluginFactoryTest.java Outdated

@@ @@ -57,7 +57,7 @@ public void testCreate() { @@
                    */
                   @Test
                   public void testCreateFail() {
-                      PluginDescriptor pluginDescriptor = mock(PluginDescriptor.class);
+                      PluginDescriptor pluginDescriptor = mock(DefaultPluginDescriptor.class);

Member

decebals Nov 1, 2017

No need to change. I feel that it's from the refactoring action

pf4j/src/test/java/org/pf4j/DefaultPluginFactoryTest.java Outdated

@@ @@ -75,7 +75,7 @@ public void testCreateFail() { @@
                    */
                   @Test
                   public void testCreateFailNotFound() {
-                      PluginDescriptor pluginDescriptor = mock(PluginDescriptor.class);
+                      PluginDescriptor pluginDescriptor = mock(DefaultPluginDescriptor.class);

Member

decebals Nov 1, 2017

No need to change. I feel that it's from the refactoring action

pf4j/src/test/java/org/pf4j/DefaultPluginFactoryTest.java Outdated

@@ @@ -93,7 +93,7 @@ public void testCreateFailNotFound() { @@
                    */
                   @Test
                   public void testCreateFailConstructor() {
-                      PluginDescriptor pluginDescriptor = mock(PluginDescriptor.class);
+                      PluginDescriptor pluginDescriptor = mock(DefaultPluginDescriptor.class);

Member

decebals Nov 1, 2017

No need to change. I feel that it's from the refactoring action

Contributor Author

josiahhaswell Nov 1, 2017

These are for testing the DefaultPluginDescriptor--are you sure you want to mock the interface and test the mock?

Member

decebals Nov 1, 2017

This test was not written by me but after I reviewed again the code I believe that original code is the right one. We need to test DefaultPluginFactory class and from this reason the author mock the code for PluginDescriptor. I don't know why don't use the real object DefaultPluginDescriptor without mock but this is another story.

pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java

@@ @@ -27,13 +27,13 @@ @@
               public class DefaultPluginManagerTest {
-                  private PluginDescriptor pd1 = null;
+                  private DefaultPluginDescriptor pd1 = null;

Member

decebals Nov 1, 2017

No need to change. I feel that it's from the refactoring action


          Fixing requested changes where possible

13605df

coveralls commented Nov 1, 2017 •

edited

Loading

Coverage decreased (-0.4%) to 56.605% when pulling 13605df on josiahhaswell:PF4J-179 into a30c05b on decebals:master.


          Being a good citizen and adding a test to help coveralls feel like th…

…e test coverage is improving

coveralls commented Nov 1, 2017 •

edited

Loading

Coverage increased (+0.2%) to 57.167% when pulling 3388472 on josiahhaswell:PF4J-179 into a30c05b on decebals:master.


          Adding more tests like a good citizen

cde9506

coveralls commented Nov 1, 2017 •

edited

Loading

Coverage increased (+1.0%) to 57.954% when pulling cde9506 on josiahhaswell:PF4J-179 into a30c05b on decebals:master.

decebals requested changes

View reviewed changes

pf4j/src/main/java/org/pf4j/ManifestPluginDescriptorFinder.java

                       return pluginDescriptor;
                   }
-                  protected PluginDescriptor createPluginDescriptorInstance() {
-                      return new PluginDescriptor();
+                  protected DefaultPluginDescriptor createPluginDescriptorInstance() {

Member

decebals Nov 1, 2017

My bad. On the returned object we call setXYZ.

pf4j/src/main/java/org/pf4j/PluginDescriptor.java Outdated

-               * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-               * See the License for the specific language governing permissions and
-               * limitations under the License.
-               */
               package org.pf4j;

Member

decebals Nov 1, 2017

Don't forget about the copyright header

pf4j/src/test/java/org/pf4j/processor/ServiceProviderExtensionStorageTest.java

		@@ -0,0 +1,29 @@
		package org.pf4j.processor;

Member

decebals Nov 1, 2017

Copyright header

pf4j/src/test/java/org/pf4j/DefaultPluginFactoryTest.java Outdated

@@ @@ -93,7 +93,7 @@ public void testCreateFailNotFound() { @@
                    */
                   @Test
                   public void testCreateFailConstructor() {
-                      PluginDescriptor pluginDescriptor = mock(PluginDescriptor.class);
+                      PluginDescriptor pluginDescriptor = mock(DefaultPluginDescriptor.class);

Member

decebals Nov 1, 2017

This test was not written by me but after I reviewed again the code I believe that original code is the right one. We need to test DefaultPluginFactory class and from this reason the author mock the code for PluginDescriptor. I don't know why don't use the real object DefaultPluginDescriptor without mock but this is another story.

Member

decebals commented Nov 1, 2017

Can you show us a snippet from your custom PluginDescriptor?
My feeling that something is wrong. Probably what you want is a custom PluginDescriptorFinder, a YamlPluginDescriptorFinder.

Contributor Author

josiahhaswell commented Nov 1, 2017 •

edited

Loading

Sure--I'm already using the changes I've made. With these changes, I can just use a DefaultPluginDescriptor instead of a new YamlPluginDescriptor. There is no way to get that info from the YAML file into the plugin descriptor that I can find without these changes.

public class YamlPluginDescriptorFinder implements PluginDescriptorFinder {
    
    
    @Override
    public boolean isApplicable(Path pluginPath) {
        final String pathName = pluginPath.toString();
        if(pathName.endsWith(".jar") || pathName.endsWith(".war") || pathName.endsWith(".zip")) {
            try {
                final JarFile jarFile = new JarFile(pluginPath.toFile());
                final Manifest manifest = jarFile.getManifest();
                if(manifest != null) {
                    final Attributes attributes = manifest.getMainAttributes();
                    
                    if (attributes != null) {
                        if("true".equals(attributes.getValue("sunshower-plugin"))) {
                            return true;
                        }
                    }
                }
            } catch (IOException e) {
                return false;
            }
        }
        return false;
    }

    @Override
    public PluginDescriptor find(Path pluginPath) throws PluginException {
        try {
            final ZipFile zipFile = new ZipFile(pluginPath.toFile());
            final ZipEntry entry = zipFile.getEntry("plugin.yml");
            if(entry != null) {
                return readEntry(zipFile, entry);
            }
            return null;
        } catch (IOException e) {
            throw new PluginException(e);
        }
    }

    private PluginDescriptor readEntry(ZipFile zipFile, ZipEntry entry) throws IOException {
        final Yaml yaml = new Yaml();
        Map load = (Map) yaml.load(zipFile.getInputStream(entry));
        return new YamlPluginDescriptor(load);
    }
}

public class YamlPluginDescriptor extends DefaultPluginDescriptor {
    
    public static final String GROUP_ID_NAME = "group";
    public static final String ARTIFACT_ID_NAME = "artifact";
    public static final String VERSION_NAME = "version";
    public static final String DESCRIPTION_NAME = "version";
    public static final String PLUGIN_CLASS_NAME = "plugin-class";
    
    
    public YamlPluginDescriptor(Map load) {
        super(
                getRequired(GROUP_ID_NAME, load),
                get(DESCRIPTION_NAME, load),
                getRequired(PLUGIN_CLASS_NAME, load),
                get(VERSION_NAME, load),
       
        );
    }

Member

decebals commented Nov 1, 2017

Your snippet looks good.


          Making requested changes

coveralls commented Nov 1, 2017 •

edited

Loading

Coverage increased (+1.0%) to 57.954% when pulling 2678871 on josiahhaswell:PF4J-179 into a30c05b on decebals:master.

decebals merged commit 5b19200 into pf4j:master

Member

decebals commented Nov 1, 2017

Thanks!

josiahhaswell deleted the PF4J-179 branch

November 1, 2017 19:16

Contributor Author

josiahhaswell commented Nov 1, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet