Simplify creation of new Version constants

[sig-daveka]

This is an enhancement request, not a bug, but creating new Version constants when there is no PredefinedVersion (e.g. for V3_7_21) is rather awkward. Callers need to create their own Version implementation that mimics the PredefinedVersion constructor and also make their own ArtifactRepository class, because the official one only handles the special case of Mac downloads for PredefinedVersions. (That could be resolved by having OfficialArtifactRespository.getArtifactPlatform() compare the version component lists when in doubt, but being enums I can't subclass GITHUB et al. to adjust that myself.) It's all possible, but it takes a bit more code than it seems like it deserves.

[AlejandroRivera]

To be honest, when someone wanted to use a version that wasn't already part of the predefined list, I assumed they'd just override the download URL (and ignore the Versions stuff), like so:

String url = "https://github.com/rabbitmq/rabbitmq-server/releases/download/v3.7.21/rabbitmq-server-generic-unix-3.7.21.tar.xz";
configBuilder.downloadFrom(new URL(url), "rabbitmq_server-3.7.21")

But simplifying things so it's easier to override the Version sounds like it could be a good idea! Would you mind drafting and submitting some code to illustrate your approach?

[sig-daveka]

I saw that, but making the OS-dependent URLs myself was scary :) The fix to OfficialArtifactReposity.getArtifactPlatform() is straight-forward. Just change the check to something like:

        if (operatingSystem == OperatingSystem.MAC_OS
                && !(version instanceof CustomVersion)
                && Version.VERSION_COMPARATOR.compare(PredefinedVersion.V3_7_18, version) <= 0) {

Exposing a version constructor is a little messier because I assume that you want to keep PredefinedVersion as an enum for backward compatibility. I've attached a couple variations on the idea. The v1 files create a new StandardVersion implementation and have the PredefinedVersion values use those constants. But duplicating all the known versions in two places felt wrong, so I created the v2 files to keep everything except the constructor down in PredefinedVersion. Those are probably the least change to the existing code. Finally I included a v3 file that could replace PredefinedVersion is you were willing to break compatibility. None of the sketches manages to use the nice trick of parsing the enum name to get the version, but maybe you see a way!

v1-PredefinedVersion.java.txt
v1-StandardVersion.java.txt
v2-PredefinedVersion.java.txt
v2-VersionImpl.java.txt
v3-PredefinedVersion.java.txt

[sig-daveka]

Another random idea came to me -- start with v2 but make VersionImpl the new CustomVersion. Then we could remove the "instanceof" clause from the getArtifactPlatform() check and change the config downloadFrom() method to something like this:

    public Builder downloadFrom(final URL downloadSource, final String appFolderName) {
      this.artifactRepository = new CustomArtifactRepository(downloadSource);
      this.version = new CustomVersion("0.0.0", null, null, null) {
		@Override
		public String getExtractionFolder() {
			return appFolderName;
		}
      };
      return this;

I like this approach best -- no new classes (although CustomVersion is now public), cleaner code in the artifact repository, and PredefinedVersion is still an enum. Here are some diffs:

v4-diffs.txt

[AlejandroRivera]

@dkaelbling, sorry about the delay. Please take a look at PR #62 (which is heavily based on your v4 code/idea) and LMK your thoughts.

[sig-daveka]

Looks great, thanks!

[AlejandroRivera]

Implemented, merged and released with v1.4.0