Repository navigation
Update AndroidX check to use regex - #188651
Julienraptor01 wants to merge 1 commit into
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. |
github-actions
Bot
added
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the isAppUsingAndroidX function in gradle.dart to use a regular expression instead of a substring check when verifying if AndroidX is enabled in gradle.properties. Feedback on this change points out that the proposed regular expression can match commented-out lines, and suggests anchoring the pattern to the start of the line with multi-line matching enabled to avoid false positives.
| return false; | ||
| } | ||
| return properties.readAsStringSync().contains('android.useAndroidX=true'); | ||
| return RegExp(r'android\.useAndroidX\s*=\s*true').hasMatch(properties.readAsStringSync()); |
There was a problem hiding this comment.
The regular expression android\.useAndroidX\s*=\s*true can incorrectly match commented-out lines in gradle.properties (for example, # android.useAndroidX=true). To avoid false positives, anchor the pattern to the start of the line using ^ and enable multi-line matching with multiLine: true.
| return RegExp(r'android\.useAndroidX\s*=\s*true').hasMatch(properties.readAsStringSync()); | |
| return RegExp(r'^\s*android\.useAndroidX\s*=\s*true', multiLine: true).hasMatch(properties.readAsStringSync()); |
There was a problem hiding this comment.
the way that it worked before allow for the same exact issue and fixing it is beyond the point of this overly simple PR
|
Closing since it's been idle for 3 weeks. Also this PR: #188372 addresses the same issue and has unit tests. |
2 participants
This check is currently broken as gradle.properties can by example have
android.useAndroidX = trueThis has been infuriating me for a while as i hate seeing warnings when there's no reason for
Pre-launch Checklist
///).