Improve consistency of hooks #486
Conversation
| @@ -49,8 +48,7 @@ public boolean check(@NonNull BeforeExecutionContext context) { | |||
| } | |||
|
|
|||
| @Override | |||
| public void action(@NonNull BeforeExecutionContext context) | |||
| throws PluginCompatibilityTesterException { | |||
There was a problem hiding this comment.
Removing an unnecessary throws statement.
| return "org.jenkins-ci.plugins".equals(model.getGroupId()) | ||
| && "jacoco".equals(model.getArtifactId()) | ||
| && "hpi".equals(model.getPackaging()); |
There was a problem hiding this comment.
Elsewhere we check group ID, artifact ID, and packaging, so do so here as well.
There was a problem hiding this comment.
elsewhere we are going the wrong way - we should just check the pluginID as we are in the business of checking plugins not artifacts, but 🤷
aka context.getPlugin().name.equals("jacoco");
There was a problem hiding this comment.
I guess we just have different philosophies. I prefer to be as paranoid as possible, possibly from my days doing kernel development.
| return "io.jenkins.plugins".equals(model.getGroupId()) | ||
| && ARTIFACT_IDS.contains(model.getArtifactId()) | ||
| && "hpi".equals(model.getPackaging()); |
There was a problem hiding this comment.
Elsewhere we check group ID, artifact ID, and packaging, so do so here as well.
| return "io.jenkins.plugins".equals(model.getGroupId()) | ||
| && ARTIFACT_IDS.contains(model.getArtifactId()) | ||
| && "hpi".equals(model.getPackaging()); |
There was a problem hiding this comment.
Elsewhere we check group ID, artifact ID, and packaging, so do so here as well.
| && "hpi".equals(model.getPackaging()); | ||
| } | ||
| return false; | ||
| VersionNumber pluginVersion = new VersionNumber(context.getPlugin().version); |
There was a problem hiding this comment.
This null check was redundant: we always set the plugin and version.
| return "org.jenkins-ci.plugins".equals(model.getGroupId()) | ||
| && "jacoco".equals(model.getArtifactId()) | ||
| && "hpi".equals(model.getPackaging()); |
There was a problem hiding this comment.
elsewhere we are going the wrong way - we should just check the pluginID as we are in the business of checking plugins not artifacts, but 🤷
aka context.getPlugin().name.equals("jacoco");
Makes these hooks more consistent with each other.