eclipse / xtext-core

xtext-core
Eclipse Public License 2.0
116 stars 96 forks source link

XtextVersionTests fails with milestone versions #1351

Closed cdietrich closed 4 years ago

cdietrich commented 4 years ago

the pattern in XtextVersionTests does not accept e.g. 2.21.0.M1

cdietrich commented 4 years ago

cc @kthoms

ArneDeutsch commented 4 years ago

In TargetPlatformProject there is a conditino that selects update repositories based on this version string:

<location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
    <unit id="org.eclipse.xtext.sdk.feature.group" version="0.0.0"/>
    «IF config.xtextVersion.isSnapshot»
        <repository location="https://download.eclipse.org/modeling/tmf/xtext/updates/nightly/"/>
    «ELSEIF config.xtextVersion.isStable»
        <repository location="https://download.eclipse.org/modeling/tmf/xtext/updates/milestones/"/>
    «ELSE»
        <repository location="https://download.eclipse.org/modeling/tmf/xtext/updates/releases/«config.xtextVersion»/"/>
    «ENDIF»
</location>

Notable: e.g. 2.9.2.beta3 is considered stable by this code whereas 2.9.2 is NOT stable (but release). Considering this I would think 2.21.0.M1 should be considered to be stable, too. Then it will load from milestones.

cdietrich commented 4 years ago

has this changed? hmmmmm

cdietrich commented 4 years ago

@ArneDeutsch

    public static void main(String[] args) {
        System.out.println(new XtextVersion("2.21.0.M1").isStable());
    }

gives true for stable

ArneDeutsch commented 4 years ago

Yes, just did the same test. But then I wonder where this ticket originates from. Seems the test works well ...

cdietrich commented 4 years ago

the test itself has a pattern. and that one fails

cdietrich commented 4 years ago

(see PR)

ArneDeutsch commented 4 years ago

Ok, as the test is quite new I would say @kthoms has missed the milestone case and your fix is ok.

cdietrich commented 4 years ago

yes but i dont know if karsten had some intention behind the test this is why i wait for the review