Use enum for package type #1609
Conversation
| TAR; | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
Why is a custom toString necessary? IIRC the enum toString prints nicely.
There was a problem hiding this comment.
I am not sure if it will return a lowercase one or an uppercase one.
|
|
||
| @Override | ||
| public String toString() { | ||
| switch (this) { |
| public static String getBaseName(String file) { | ||
| return new File(file).getName(); | ||
| } | ||
|
|
||
| public static String getPkgType(String topologyBinaryFile) { | ||
| String pkgType; | ||
| public static PackageType getPackageType(String topologyBinaryFile) { | ||
| String basename = FileUtils.getBaseName(topologyBinaryFile); | ||
| if (FileUtils.isOriginalPackagePex(basename)) { |
There was a problem hiding this comment.
This if statement can be replaced with a static factory method on the enum. Something like this:
public enum PackageType {
PEX,
JAR,
TAR
public static PackageType getType(String filename) {
// parse extension
return PackageType.valueOf(extension);
}
}
Actually, at that point you no longer even need FileUtils.getPackageType anymore, since callers can just get the type from the enum
There was a problem hiding this comment.
yes that will be easier. I realized that I am just cargo-culting.
| @@ -186,7 +186,7 @@ protected String formatJavaOpts(String javaOpts) { | |||
| auroraProperties.put("COMPONENT_RAMMAP", Runtime.componentRamMap(runtime)); | |||
| auroraProperties.put("COMPONENT_JVM_OPTS_IN_BASE64", | |||
| formatJavaOpts(TopologyUtils.getComponentJvmOptions(topology))); | |||
| auroraProperties.put("TOPOLOGY_PACKAGE_TYPE", Context.topologyPackageType(config)); | |||
| auroraProperties.put("TOPOLOGY_PACKAGE_TYPE", Context.topologyPackageType(config).toString()); | |||
There was a problem hiding this comment.
We don't want to rely on toString() here since that should only ever really be used for logging. If we need a string better to call .name() on the enum, or create a value field in the enum that is defined as part of the enum (i.e., TAR("tar")).
| } | ||
|
|
||
| public static PackageType getPackageType(String topologyBinaryFile) { | ||
| String basename = FileUtils.getBaseName(topologyBinaryFile); |
There was a problem hiding this comment.
We can remove ~20 lines of code and just use guava Files class here:
String extension = Files.getFileExtension(topologyBinaryFile)
return PackageType.valueOf(extension.toUpperCase();
| @@ -212,7 +212,7 @@ private SchedulerUtils() { | |||
| commands.add(Context.systemConfigSandboxFile(config)); | |||
| commands.add(Runtime.componentRamMap(runtime)); | |||
| commands.add(SchedulerUtils.encodeJavaOpts(TopologyUtils.getComponentJvmOptions(topology))); | |||
| commands.add(Context.topologyPackageType(config)); | |||
| commands.add(Context.topologyPackageType(config).toString()); | |||
There was a problem hiding this comment.
better to use .name() than toString. Also check on the cases, think both return caps.
| Assert.assertEquals(PackageType.getPackageType(pexFile), PackageType.PEX); | ||
| } | ||
|
|
||
| @Test(expected = RuntimeException.class) |
There was a problem hiding this comment.
Is this RuntimeException or a subclass? If a subclass we should expect that instead.
| JAR, | ||
| TAR; | ||
|
|
||
| public static PackageType getPackageType(String topologyBinaryFile) { |
There was a problem hiding this comment.
We should rename topologyBinaryFile to something more generic like filename.
There was a problem hiding this comment.
how about packageFile, since this class is PackageType? filename is too general
|
👍 once CI passes |
|
👍 |
This PR refactors so that enum is used for topology package type. It addresses discussions at #1599 (comment)
Follow-up of #1599
cc @billonahill @mycFelix