Skip to content
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

PluginDescriptor can't be extended #107

Closed
DimaSol opened this issue Aug 9, 2016 · 10 comments
Closed

PluginDescriptor can't be extended #107

DimaSol opened this issue Aug 9, 2016 · 10 comments

Comments

@DimaSol
Copy link

DimaSol commented Aug 9, 2016

Hi,
I'm not sure if this is in purpose but there is no way to extend the PluginDescriptor to support custom properties.
I'm extending ManifestPluginDescriptorFinder to create a CustomPluginDescriptor but createPluginDescriptor is the one that is creating the PluginDescriptor so I have to fully override the createPluginDescriptor and call all the core properties setters but the setters are private to the package so it becames very not pretty...
I would expect the PluginDescriptorFinder interface or ManifestPluginDescriptorFinder class to have a public PluginDescriptor createDescriptorInstance method that will return a new instance (and this one can be simply overriden by any PluginDescriptorFinder inplementation), in such way in the extending createPluginDescriptor method a super.createPluginDescriptor can be called to init all core properties and getting back the extending custom PluginDescriptor that can be further enhanced.

@decebals
Copy link
Member

decebals commented Aug 9, 2016

I'm not sure if this is in purpose but there is no way to extend the PluginDescriptor to support custom properties.

I don't understand why (and where) you need custom properties.

I'm extending ManifestPluginDescriptorFinder to create a CustomPluginDescriptor but createPluginDescriptor is the one that is creating the PluginDescriptor so I have to fully override the createPluginDescriptor and call all the core properties setters but the setters are private to the package so it becames very not pretty...
I would expect the PluginDescriptorFinder interface or ManifestPluginDescriptorFinder class to have a public PluginDescriptor createDescriptorInstance method that will return a new instance (and this one can be simply overriden by any PluginDescriptorFinder inplementation), in such way in the extending createPluginDescriptor method a super.createPluginDescriptor can be called to init all core properties and getting back the extending custom PluginDescriptor that can be further enhanced.

I can extract the PluginDescriptor pluginDescriptor = new PluginDescriptor(); line from the ManifestPluginDescriptorFinder.createPluginDescriptor(Manifest manifest) method in a protected method createPluginDescriptorInstance and you can use the variant with super.createPluginDescriptor.
Until this issue I didn't found a reason why someone needs to create a custom PluginDescriptor instance but maybe you can give me a good reason to do this.

decebals added a commit that referenced this issue Aug 10, 2016
@DimaSol
Copy link
Author

DimaSol commented Aug 10, 2016

Thank you for the reply.

You are giving the ability to configure my PluginDescriptorFinder by providing a protected createPluginDescriptorFinder method and one of the things I want my descriptor finder to do is to parse extra custom attributes - I think it will only by consistent to support also an extension of the PluginDescriptor.

An example for such need is: my plugins are holding extensions for JSF application and I need the ability to define in the manifest the components that are being added to specific panels, JSF managed beans and maybe hinerante domain objects.

@decebals
Copy link
Member

I made the modification and you can do your job but to be honest with you I don't understand why you want to use PluginDescriptor for a such task. You can move all this logic with read the Manifest and extract some metadata in a AcmePlugin (your base plugin class) that extends Plugin class.

@DimaSol
Copy link
Author

DimaSol commented Aug 10, 2016

You are right and this is how I've implemented it up until now, but such an implementation leads to parsing the manifest again what I think is not pretty - If we already have a utility that is doing it, I would like to use it instead of re-inventing it again...
Anyway, thank you for modifieng it.

@decebals
Copy link
Member

Can you close this issue, of course if everything is OK for you?

@DimaSol
Copy link
Author

DimaSol commented Aug 12, 2016

Sure, when this fix planned to be released?

@DimaSol DimaSol closed this as completed Aug 12, 2016
@decebals
Copy link
Member

Sure, when this fix planned to be released?

I wait a feedback for #108 and if everything is OK a can release a next version.

@DimaSol
Copy link
Author

DimaSol commented Aug 22, 2016

Hi @decebals,
Can you kindly share an estimation when is the next version going to be released?

@decebals
Copy link
Member

Probably tonight or tomorrow morning.

@decebals
Copy link
Member

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants