Readability change to flutter.groovy, align on null assignment, reduce unused scope for some methods, apply static where possible
#157471
Conversation
github-actions
bot
added
the
tool
|
windows tests fail , mac tests pass. |
|
test-exempt: code refactor with no semantic change |
|
@AbdeMohlbi I am going to give this a pass but can you update the title and pr description to give a more precise description of what is being refactored and why? It is helpful when reviewing all commits post release and also gives your code reviews more context. |
| } | ||
| if (num2 > num1) { | ||
| return version2 | ||
| List<Integer> version1Tokenized = version1.tokenize(".")*.toInteger() |
There was a problem hiding this comment.
Semantically I don't think this is the same. Specifically I think a number like 8.6-rc-2 would crash on the first 2 lines.
There was a problem hiding this comment.
yep it should be List<String> but still this does not resolve cases like 8.6-rc-2 , like the TODO above says.
hmmm , should i work for an overall fix for this or just let it be list ?
There was a problem hiding this comment.
Most of this pr is good, lets remove this change and land the rest. Then you can either tackle this or not based on what you prefer.
There was a problem hiding this comment.
currenlty i'm seeing a list of all gradle versions (3160 as of today) ,i see that there is only the special case of rc , but i have no idea how to compare them for exemple is 8.7-rc-1 with 8.8 , i guess 8.8 is newer
There was a problem hiding this comment.
well i found some script , tweaked it a bit and it seam to wrok :
(could u kindly confirm that this the intended behavior ?)
def compareGradleVersions(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
}
// like -rc-1 cass
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
}
}
return version1
// If both are same return one of thm
}
def comparePreReleaseIdentifiers(String v1Part, String v2Part) {
// i tested "rc-1" vs "rc-2" worked (rc-2)
// i tested "8.7-rc-1" vs "8.7" worked (8.7)
// i tested "8.7-rc-1" vs "8.7.2" worked (8.7.2)
// i tested "8.7.1" vs ""8.7.2"" worked (8.7.2)
def v1PreRelease = v1Part.replaceAll("\\D", "")
def v2PreRelease = v2Part.replaceAll("\\D", "")
return v1PreRelease < v2PreRelease
}
def newerVersion = compareGradleVersions("8.7.1", "8.7.2")
println "The newer version is: $newerVersion"
No problem |
reidbaker
left a comment
reidbaker
left a comment
There was a problem hiding this comment.
Unlike some of the other pr's this one has a mix of lint style fixes and some opinion style fixes.
FWIW I think moving from if (something == null) set a default to using the ?: syntax is a bit more readable.
Adding static where possible is a good change.
reducing the scope of methods that dont need to be public is ok but I need to do a pass to make sure we dont actually depend on those methods someplace else.
|
Test-exemption: This pr should not land with any behavioral changes. If there are behavioral changes then tests need to be added. |
Well the file contains multiple instances of both , i tried unify the usage to ?: |
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
Yeah sorry I this was mostly a list of things I liked in the pr. |
AbdeMohlbi
changed the title
flutter.groovyflutter.groovy to use suitable modifiers
reidbaker
changed the title
flutter.groovy to use suitable modifiersflutter.groovy, align on null assignment, reduce unused scope for some methods, apply static where possible
|
related #158020 |
reidbaker
added
the
autosubmit
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
flutter/flutter@8591d0c...29d40f7 2024-11-05 [email protected] increase subsharding for `Windows build_tests` from 8 to 9 (flutter/flutter#158146) 2024-11-05 [email protected] Reland2: Revert "Revert "Add a warning/additional handlers for parsing`synthetic-package`."" (flutter/flutter#158184) 2024-11-05 [email protected] Reland1: "Revert "Add and plumb `useImplicitPubspecResolution` across `flutter_tools`."" (flutter/flutter#158126) 2024-11-05 [email protected] Roll Packages from 796afa3 to 7219431 (11 revisions) (flutter/flutter#158179) 2024-11-05 [email protected] Make native asset integration test more robust, thereby allowing smooth auto-update of packages via `flutter update-packages` (flutter/flutter#158170) 2024-11-05 [email protected] Readability change to `flutter.groovy`, align on null assignment, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471) 2024-11-05 [email protected] Roll Flutter Engine from 7207a8fbec93 to f56401062e42 (1 revision) (flutter/flutter#158169) 2024-11-05 [email protected] Add test for `raw_scrollbar.shape.0.dart` (flutter/flutter#158094) 2024-11-05 [email protected] Roll Flutter Engine from 418609dd5b58 to 7207a8fbec93 (1 revision) (flutter/flutter#158156) 2024-11-05 [email protected] Refactor DropdownMenu tests (flutter/flutter#157913) 2024-11-05 [email protected] Roll Flutter Engine from 6271a92a376f to 418609dd5b58 (3 revisions) (flutter/flutter#158152) 2024-11-05 [email protected] Marks Linux_pixel_7pro flavors_test to be flaky (flutter/flutter#156956) 2024-11-05 [email protected] Further remove web-only considerations that are no longer necessary (flutter/flutter#158143) 2024-11-05 [email protected] Add optional parameter to FlutterTesterDevices. (flutter/flutter#158133) 2024-11-05 [email protected] Roll Flutter Engine from 75acceedca41 to 6271a92a376f (2 revisions) (flutter/flutter#158148) 2024-11-05 [email protected] Extract and restore a test that a blank native assets project still builds (flutter/flutter#158141) 2024-11-04 [email protected] Remove references to the HTML renderer in public docs. (flutter/flutter#158035) 2024-11-04 [email protected] Roll Flutter Engine from f880b56b6ede to 75acceedca41 (1 revision) (flutter/flutter#158137) 2024-11-04 [email protected] Fix `WidgetStateProperty` documentation (flutter/flutter#154298) 2024-11-04 [email protected] Roll Flutter Engine from 25c7e471e2ef to f880b56b6ede (5 revisions) (flutter/flutter#158132) 2024-11-04 [email protected] Roll Flutter Engine from 05cb5d7f7939 to 25c7e471e2ef (12 revisions) (flutter/flutter#158127) 2024-11-04 [email protected] Remove use_modular_headers! from Swift Podfiles (flutter/flutter#156257) 2024-11-04 [email protected] Disable failing native assets test (flutter/flutter#158119) 2024-11-04 [email protected] Fix `NestedScrollView` inner position logic (flutter/flutter#157756) 2024-11-04 [email protected] Add benchmarks for single-threaded Skwasm. (flutter/flutter#158027) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
…ent, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
4 participants
removed public modifier from this methods :
getVersionCode,getVersionName.add static to :
pluginSupportsAndroidPlatform,buildGradleFile,settingsGradleFilegetCompileSdkFromProject,getAssembleTaskrefactor
==nullusage to:?to unify the usagesee #147122 for context
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.