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

Add support for includeLaunchers in product file #58

Merged
merged 4 commits into from
Feb 25, 2018

Conversation

hacki11
Copy link
Contributor

@hacki11 hacki11 commented Feb 24, 2018

The following line is copied from a example product file:
<product uid="com.foo.plugin.product" id="com.foo.plugin.product" application="org.eclipse.ui.ide.workbench" version="1.0.0" useFeatures="false" includeLaunchers="false">

PdeBuildTask will now extract this property and adds it to PdeBuildProperties.
A launcher is now excluded or included in the PDE export.

@nedtwigg would you mind doing a code review?

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Very minor change then feel free to merge.


static Properties extractProperties(String[] lines) {
Properties props = new Properties();
for (String line : lines) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Map<String, String> props = new LinkedHashMap<>() instead of Properties?

@@ -89,8 +90,11 @@ void setup(File destinationDir, PdeBuildProperties props, List<SwtPlatform> plat

// now create the sanitized product file
File productFile = productPluginDir.toPath().resolve(productFileWithinPlugin).toFile();
productFileLines = ProductFileUtil.readLines(productFile);
ProductFileUtil.extractProperties(productFileLines).forEach((key, value) -> props.setProp(key.toString(), value.toString()));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After making the Map<String, String> change, this could be .forEach(props::setProp)

@hacki11
Copy link
Contributor Author

hacki11 commented Feb 25, 2018

good catch, i applied them accordingly

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

Successfully merging this pull request may close these issues.

None yet

2 participants