java9-modularity / gradle-modules-plugin

This Gradle plugin helps working with the Java Platform Module System
https://javamodularity.com/
MIT License
233 stars 36 forks source link

Proposal: More convenience methods in `modularity` extension #83

Open tlinkowski opened 5 years ago

tlinkowski commented 5 years ago

@paulbakker, let me know if you'd be interested in accepting PRs for the following:

1. Adding convenience methods modularity.addModules, etc.

ModuleOptions for various tasks (compileJava, compileTestJava, test, javadoc, run) currently share the following four config options: addModules, addReads, addExports, and addOpens. However, all of them have to be set separately for every task.

I propose adding convenience methods to ModularityExtension that would append to those config options for all applicable tasks.

Signatures:

interface ModularityExtension {
  // ...
  void addModules(String... moduleNames);
  void addReads(Map<String, String> addReads);
  void addExports(Map<String, String> addExports);
  void addOpens(Map<String, String> addOpens);
}

For example modularity.addModules 'java.sql' would add module java.sql to all the tasks.

This is related to #76.

2. Adding convenience method modularity.patchModules

I suggest adding:

interface ModularityExtension {
  // ...
  void patchModules(Map<String, String> patchModules);
}

For consistency, I changed the signature from List<String> (where strings are separated with =) to Map<String, String> so that it aligns with addReads, addExports, and addOpens (introduced by #74).

This would let us call:

modularity.patchModules [
        'java.annotation': 'jsr305-3.0.2.jar'
]

instead of

patchModules.config = [
        "java.annotation=jsr305-3.0.2.jar"
]

Moreover, since javac's --patch-module supports patching multiple JARs into a module (separated with :), this method would allow such notation. For example:

modularity.patchModules [
        'java.annotation': 'jsr305-3.0.2.jar:some-more-javax-annotations.jar'
]

would map to

patchModules.config = [
        "java.annotation=jsr305-3.0.2.jar",
        "java.annotation=some-more-javax-annotations.jar"
]

I also propose deprecating direct patchModules extension usage.

paulbakker commented 5 years ago

This would add this as an option, while the current behavior is still supported as well, correct? Looks like a great idea. I originally thought about doing this as the default behavior, but wasn't sure at the time if that would be flexible enough. Having both options would be really good.

We should make it very clear in the doc what the recommended approach is, it's potentially confusing (but not wrong) that there are two ways to achieve the same thing. I think the new approach should be the preferred approach, since that usually what you need anyway.

tlinkowski commented 5 years ago

This would add this as an option, while the current behavior is still supported as well, correct?

Yes, definitely:

ModularityExtension won't even be stateful — it'll just set the options in the tasks' moduleOptions.

We should make it very clear in the doc what the recommended approach is [...]. I think the new approach should be the preferred approach, since that usually what you need anyway.

Agree.


It seems as if your comments only concerned point no. 1, am I right? Do you have any thoughts about point no. 2? Or can I just assume you agree with it too?