SWITCHYARD-2959 - ensuring that previous version is in SY version #634
Conversation
| _availableVersions.add(defaultVersion); | ||
| } | ||
|
|
||
| // for SWITCHYARD-2959 - add previous version as well |
There was a problem hiding this comment.
I prefer not setting JIRA number, would be better to extract a method with a name like ensureDefaultAndPreviousVersionAvailable
There was a problem hiding this comment.
Is that a stylistic thing (not setting JIRA # in the comment)? Because you didn't comment on it in the changes for #637
If it is a style thing and something we want to achieve going forward, I'm fine with it - but I personally like leaving breadcrumbs back to the reasons behind why certain changes were made. The goal of comments is to make the code more readable and this just gives us more information. Just my opinion of course.
There was a problem hiding this comment.
As for popping this out to another method, I'm good with that - it holds with the principles of "Building Maintainable Software" pretty closely.
bfitzpat
force-pushed
the
SWITCHYARD-2959
branch
from
d353efa
to
05a8cf9
Compare
|
@apupier I have updated this PR per your suggestions. Had to do a bit of refactoring (probably more than I needed, but it made sense to move more things back to M2EUtils), but I moved the new code into a new method and added a new test. Works locally and tests all pass. Just waiting for the PR build |
| final ArtifactVersion defaultVersion = M2EUtils.parseVersion(NewSwitchYardProjectWizard.DEFAULT_RUNTIME_VERSION); | ||
| final ArtifactVersion previousVersion = M2EUtils.parseVersion(NewSwitchYardProjectWizard.PREVIOUS_RUNTIME_VERSION); | ||
|
|
||
| assertTrue("Versions list missing default SwitchYard Version", versions.contains(defaultVersion)); |
There was a problem hiding this comment.
Optional:
I like to use AssertJ to check for collections.
For instance here, it will be something like:
assertThat(versions).as("Versions list missing default SwitchYard Version").contains(defaultVersion);
but maybe a matter of taste
There was a problem hiding this comment.
I'm ok with the older approach, but I see your point. I'll look at more prescriptive assert statements as we go forward but don't see this as necessary for now.
| @@ -353,4 +358,14 @@ protected void execute(IProgressMonitor monitor) throws CoreException { | |||
|
|
|||
| } | |||
|
|
|||
| public void testVersionsListHasDefaultAndPreviousVersions() throws Exception { | |||
There was a problem hiding this comment.
optional: might worth adding a second test to ensure that they are not added twice
There was a problem hiding this comment.
As you point out below, it's a set, so they're unique.
| public static Set<ArtifactVersion> ensureDefaultAndPreviousVersionAvailable(final Set<ArtifactVersion> versions) { | ||
| // add default version | ||
| final ArtifactVersion defaultVersion = parseVersion(NewSwitchYardProjectWizard.DEFAULT_RUNTIME_VERSION); | ||
| if (!versions.contains(defaultVersion)) { |
There was a problem hiding this comment.
Just realizing now: if it is a Set we don't need to check if it is contained inside first
drop-down list. In this case, 2.0.0.Final - also refactored some code for a bit easier testing - added new test - updated to remove if (!contains) statements (unneeded since we're adding to a set, which guarantees uniqueness)
bfitzpat
force-pushed
the
SWITCHYARD-2959
branch
from
05a8cf9
to
aa2fb95
Compare
| final Set<ArtifactVersion> versions = filterSwitchYardVersions(resolveSwitchYardVersionRange(monitor)); | ||
| return versions; | ||
| } catch (CoreException e) { | ||
| final Set<ArtifactVersion> versions = new LinkedHashSet<ArtifactVersion>(); |
There was a problem hiding this comment.
optional: directly return new LinkedHashSet();
|
Thanks Aurelien |
drop-down list. In this case, 2.0.0.Final