Simplify plugin proposal #53
Conversation
The `jar_configured` and `war_configured` checks never evaluate to true in the test cases. I think this code may have been added while exploring solutions to Dispader#16. Since the checks never run, the class variables and assignments have also been removed.
Since War depends on Java/Jar only one check is necessary. Checking for both War and Jar is redundant. This also makes use of the `withType` method.
This is just a proposal. I think ideally the plugin would have little or no logic—like a thin controller. Some of the logic that was here I think is unnecessary. For example: - `Version.warningText` is not valid code - Explicitly declaring a project.version that's different from the source control tag is weird if you're opting into this plugin - project.manifesto? seem necessary. If the project.manifesto extension doesn't exist, this plugin is fatally broken. I think there is value in wrapping logic around attributes that might be null, but maybe we should introduce another object to contain the logic that can be tested in isolation. Thoughts?
There was a problem hiding this comment.
Bref: I like the proposed changes here.
I'll run down my thoughts on the examples given above, just so we can be sure we're talking about the same stuff.
- Version.warningText` is not valid code
- This one I don't understand: there's a
Version.getWarningText()defined. Whether we want to use it? I could get behind not, but we probably should remove the method if we drop it here.
- This one I don't understand: there's a
- Explicitly declaring a project.version that's different from the source control tag is weird if you're opting into this plugin
- Yup. Agreed.
- project.manifesto? seem necessary. If the project.manifesto extension doesn't exist, this plugin is fatally broken.
- If you mean that we're creating it ourselves in line 11, agreed.
- "wrapping logic... might be null"
- Yup, that's my biggest concern.
I really like the spirit of this Pull, though— this is a damned fine proposal.
| @@ -12,7 +11,7 @@ class ManifestoPlugin implements Plugin<Project> { | |||
| project.extensions.create('manifesto', ManifestoPluginExtension, project) | |||
|
|
|||
| project.plugins.whenPluginAdded { plugin -> | |||
| project.tasks.findAll { ( it instanceof Jar || it instanceof War ) }.each { | |||
| project.tasks.withType(Jar).each { | |||
There was a problem hiding this comment.
Hey @cdinger — I don't think the change here does what you think, but it's a really awesome change nonetheless.
This code isn't checking that the war plugin or jar plugin is loaded, but modifying configurations for the tasks that archive things up.
However — the War task actually does extend the Jar task — and this change not only means we don't pick and choose specific archives, but makes the plugin capable of configuring manifests for anything inheriting from Jar.
💯
| String vendor, vendor_id, url | ||
| String vendor = "" | ||
| String vendor_id = "" | ||
| String url = "" |
There was a problem hiding this comment.
I can see defaulting the values here. Could we make this non-interpolated (single-quoted) Strings? Here they're (double-quoted) GStrings, which is popping a Codenarc violation.
| 'Implementation-URL' : project.manifesto.url, | ||
| 'Implementation-Version' : version.implementation, | ||
| 'Implementation-Timestamp': new Date() | ||
| ] |
There was a problem hiding this comment.
I like this a lot, and think it's way less chatty with the log messages, and I'm loving the syntax here. The only thing that gives me pause is whether things like project.name will come back as empty or null. If we're not populating manifests with null, I'm way on board with this.
|
|
||
| attributes('Specification-Title': project.name) | ||
|
|
||
| if ( project.manifesto?.vendor ) { |
There was a problem hiding this comment.
project.manifesto? here was to guard against when folks were using the default configuration (not adding any kind of manifesto config in their build, and just relying on the defaults. I... think it still serves that purpose? If we don't NPE on the new code starting around line 21 when there's no manifesto { } in a build, I'm way good losing it there.
There was a problem hiding this comment.
Oh, I'm sorry— I'm seeing the source of confusion now. (Now that I read the commit comment carefully, which I should have done in the first place.)
The project.manifesto? reference here doesn't refer to the plugin at all— it's the configuration object that we create above to allow folks to configure things about the plugin in their build (manifesto { } in their build files). We might want to lose this, but we're trying to null-safe dereferencing the Plugin itself, but the configuration object that may or may not have gotten added to the project.
| @@ -10,67 +10,23 @@ class ManifestoPlugin implements Plugin<Project> { | |||
|
|
|||
| project.extensions.create('manifesto', ManifestoPluginExtension, project) | |||
|
|
|||
| def version = new Version() | |||
| project.version = version.version | |||
There was a problem hiding this comment.
This change wigs me out a little bit... it looks like it gets run only at project configuration time (not after the project's been evaluated, like the stuff in thewhenPluginAdded code below. (whenPluginAdded is no way to execute that code by the way— it's just awful, and should be in in some kind of afterEvaluate block: I just couldn't get that to work because I'm really awful at writing Gradle plugins, apparently.) Point for this comment is that I really wonder if the version will get updated when folks perform builds with something like the Gradle Daemon running— I dunno that they won't, but it seems like we're getting version information when this plugin is added, rather than when the Jar tasks are evaluated.
This is just a proposal. I think ideally the plugin would have little or no logic—like a thin controller.
Some of the logic that was here I think is unnecessary. For example:
Version.warningTextis not valid codeproject.versionthat's different from the source control tag is weird if you're opting into this pluginproject.manifesto?seem necessary. If theproject.manifestoextension doesn't exist, this plugin is fatally broken.I think there is value in wrapping logic around attributes that might be null, but maybe we should introduce another object to contain the logic that can be tested in isolation.
Thoughts?