Extract Regions from FeaturesServiceImpl#301
Extract Regions from FeaturesServiceImpl#301cschneider wants to merge 0 commit intoapache:masterfrom
Conversation
| * to allow this bundle to be stopped and still allow the deployment to | ||
| * take place. | ||
| */ | ||
| private final BundleContext systemBundleContext; |
There was a problem hiding this comment.
Why the removal of this javadoc ? It looks important to me.
There was a problem hiding this comment.
I moved the comment to Regions as I think the functionality refered by the comment is also moved. The remaining use of systemBundleContext is in getDeploymentState and I had the impression the comment does not match the usage there.. btw. as only one method uses systemBundleContext we can maybe move it out of FeaturesServiceImpl in a later refactoring.
| boolean configCfgStore) { | ||
| this.bundle = bundle; | ||
| this.bundleContext = bundleContext; | ||
| this.bundleContext = bundle == null ? null : bundle.getBundleContext(); |
There was a problem hiding this comment.
I'd rather keep the bundleContext as a parameter.
The reason is that bundle.getBundleContext() can possibly throw an exception.
|
|
||
| void saveState() throws IOException; | ||
|
|
||
| RegionDigraph getGraph() throws BundleException; |
There was a problem hiding this comment.
Maybe rename to getDigraphCopy() to better reflect what it actually does ?
| import org.osgi.resource.Resource; | ||
| import org.osgi.resource.Wire; | ||
|
|
||
| public interface Regions { |
There was a problem hiding this comment.
Regions does not sound a very good name to me... Too vague. What about something like RegionInstallationSupport ? Another thought ?
There was a problem hiding this comment.
I was also unsure. Not sure if RegionInstallationSupport is better but if you prefer it that is fine for me.
|
|
||
| void saveState(State state); | ||
| void persistResolveRequest(DeploymentRequest request) throws IOException; | ||
| void installFeature(Feature feature) throws IOException, InvalidSyntaxException; |
There was a problem hiding this comment.
I think it would make sense to move the installFeature too, as the DeployCallback would then only contain state and listeners management.
There was a problem hiding this comment.
I think installFeature is very similar to uninstallFeature. So we would need to move both. If you look what they depend on there would not be much left in FeaturesServiceImpl.
I have some ideas how to improve the situation but they would require a bigger redesign.
|
|
||
| public FeaturesServiceImpl(Bundle bundle, | ||
| BundleContext bundleContext, | ||
| BundleContext systemBundleContext, |
There was a problem hiding this comment.
Can me split out the removal of this constructor in a different commit ?
There was a problem hiding this comment.
Why would you like it to be separate?
There was a problem hiding this comment.
Yeah, smaller commits are easier to review imho
| import static java.util.jar.JarFile.MANIFEST_NAME; | ||
| import aQute.bnd.osgi.Macro; | ||
| import aQute.bnd.osgi.Processor; | ||
|
|
There was a problem hiding this comment.
Can we avoid reordering imports ?
There was a problem hiding this comment.
I will try to avoid it. Sometimes I tend to press Ctrl-Shift-O :-)
cschneider
force-pushed
the
refactoring-feature
branch
from
4b5f264 to
a1ead3d
Compare
cschneider
force-pushed
the
refactoring-feature
branch
from
a1ead3d to
2e8c6ee
Compare
2 participants
No description provided.