support semantic versions with snapshots (e.g. 1.3.0-alpha) #701
Conversation
artoonie
force-pushed
the
feature/issue-700_support-semantic-versioning
branch
from
68f69df
to
0cca150
Compare
There are a lot with nicer APIs, but this one already exists in the codebase, so it's nice that we don't have to add any dependencies for it :) |
artoonie
force-pushed
the
feature/issue-700_support-semantic-versioning
branch
from
31b7415
to
a1cdeee
Compare
| } | ||
|
|
||
| return isNewer; | ||
| public static boolean isVersionNewer(String version1, String version2) { |
There was a problem hiding this comment.
This is all kind of making me wonder if checkConfigVersionMatchesApp is actually doing anything besides logging?? It doesn't seem like it's stopping the code from running, so I'm a bit confused by this. Would you mind giving checkConfigVersionMatchesApp a sanity check?
There was a problem hiding this comment.
It doesn't do anything besides log. Based on the comments, this isn't a problem in the GUI because the GUI tries to migrate versions without any care as to the existing version (which #706 should address). For now, all it does is spit out a SEVERE and continue tabulation.
There was a problem hiding this comment.
Yeah, this is kind of weird, and I wonder if this code was removed unintentionally... in the past I think we had it block tabulation if the config file version was newer than the tabulator version. Does that make sense to you?
Conversely, if the config file version is older than the tabulator version, I'd think attempting to run via the CLI should block it and say "you need to migrate this via the GUI". What do you think?
@tarheel @chughes297 do either of you have any idea why we apparently don't block tabulation in either of these cases?
There was a problem hiding this comment.
This PR might be good to go, depending on what @chughes297 and / or @tarheel think about this comment.
There was a problem hiding this comment.
I'm not sure about the history, but I think I agree that 1) it should fail if the config is newer than the app and 2) it should warn but continue and try to auto-migrate if the app is newer than the config.
There was a problem hiding this comment.
Right on, agreed. Opened #712 for this.
| } | ||
|
|
||
| return isNewer; | ||
| public static boolean isVersionNewer(String version1, String version2) { |
There was a problem hiding this comment.
Yeah, this is kind of weird, and I wonder if this code was removed unintentionally... in the past I think we had it block tabulation if the config file version was newer than the tabulator version. Does that make sense to you?
Conversely, if the config file version is older than the tabulator version, I'd think attempting to run via the CLI should block it and say "you need to migrate this via the GUI". What do you think?
@tarheel @chughes297 do either of you have any idea why we apparently don't block tabulation in either of these cases?
| } | ||
|
|
||
| return isNewer; | ||
| public static boolean isVersionNewer(String version1, String version2) { |
There was a problem hiding this comment.
Right on, agreed. Opened #712 for this.
closes #700