-
Notifications
You must be signed in to change notification settings - Fork 29.7k
update mostRecentSemanticVersion to handle strings like "8.6-rc-2"
#158020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
i have no idea how to write tests for this one |
|
@gmackall I think this is not unit testable given our current structure. Can you confirm if that is true? |
|
Something i noticed is that this function does compare the ndk versions of two plugins and AFAIK there is no ndk version that have letters in it , i'm i right ? |
|
this seems related : flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts Lines 331 to 364 in 41c273f
|
|
using groovy online compiler, u can test the code : static String mostRecentSemanticVersion(String version1, String version2) {
def v1Parts = version1.tokenize('.-')
def v2Parts = version2.tokenize('.-')
for (int i = 0; i < Math.max(v1Parts.size(), v2Parts.size()); i++) {
def v1Part = i < v1Parts.size() ? v1Parts[i] : '0'
def v2Part = i < v2Parts.size() ? v2Parts[i] : '0'
def v1Num = v1Part.isNumber() ? v1Part.toInteger() : 0
def v2Num = v2Part.isNumber() ? v2Part.toInteger() : 0
if (v1Num != v2Num) {
return v1Num > v2Num ? version1 : version2
}
if (v1Part.isNumber() && !v2Part.isNumber()) {
return version1
} else if (!v1Part.isNumber() && v2Part.isNumber()) {
return version2
} else if (v1Part != v2Part) {
return comparePreReleaseIdentifiers(v1Part, v2Part) ? version1 : version2
}
}
// If versions are equal, return the longest version string
return version1.length() >= version2.length() ? version1 : version2
}
static boolean comparePreReleaseIdentifiers(String v1Part, String v2Part) {
def v1PreRelease = v1Part.replaceAll("\\D", "")
def v2PreRelease = v2Part.replaceAll("\\D", "")
return v1PreRelease < v2PreRelease
}
println mostRecentSemanticVersion("1.2", "1.2.0") // Output: 1.2.0
println mostRecentSemanticVersion("1.0", "1") // Output: 1.0
println mostRecentSemanticVersion("1.2.0-alpha", "1.2") // Output: 1.2
println mostRecentSemanticVersion("1.2.3", "1.2.3") // Output: 1.2.3
println mostRecentSemanticVersion("1.2.3-beta", "1.2.3") // Output: 1.2.3
println mostRecentSemanticVersion("1.2.3", "1.2.3.4") // Output: 1.2.3.4
println mostRecentSemanticVersion("rc-2", "rc-1") // Output: rc-2
println mostRecentSemanticVersion("8.7-rc-1", "8.7") // Output: 8.7
println mostRecentSemanticVersion("8.7-rc-1", "8.7.2") // Output: 8.7.2
println mostRecentSemanticVersion("8.7.2", "8.7.1") // Output: 8.7.2 |
|
Marking as draft since pr description says WIP |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
fair, i forget to change it . |
|
@AbdeMohlbi getting this on our review queue. @jesswrd will take a first pass. |
| int num2 = version2Tokenized[i].toInteger() | ||
| if (num1 > num2) { | ||
| return version1 | ||
| static String mostRecentSemanticVersion(String version1, String version2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should either define the input space to be only Major.Minor.Patch in the above doc, just to be clear that we don't care about cases like 8.7-rc-2 vs 8.7.0.1, or otherwise handle them in way that compares initially based on a tokenization of . (i.e., here 7-rc-2 in the former gets compares to 7 in the latter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the late response i have some issues with github , so i'm still a bit confused now , shoud it do something like that now ?
version1 |
version2 |
Result |
|---|---|---|
1.2.3 |
1.2.4 |
1.2.4 |
1.2.3-rc-1 |
1.2.3 |
1.2.3 |
1.2.3-beta |
1.2.3-alpha |
1.2.3 |
1.2.3.1 |
1.2.3 |
1.2.3 |
8.7-rc-2 |
8.7.0.1 |
8.7.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply - I made that comment during a triage meeting, and on second reading the comparison method seems good to me
Co-authored-by: Gray Mackall <[email protected]>
gmackall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! LGTM
|
You may need to update the branch, as it looks like the |
updated mostRecentSemanticVersion func to handle strings like "8.6-rc-2" .
see :
flutter/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy
Lines 815 to 824 in 4100601
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.