-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[tool] Change gradle-check logic to enforce alignment of java versions and a minimum (17) #10206
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
[tool] Change gradle-check logic to enforce alignment of java versions and a minimum (17) #10206
Conversation
…va version alignment
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.
LGTM.
I have an optional suggestion for restructuring; feel free to ignore it if you disagree that it's simpler.
'$indentation${minimumJavaVersionError.split('\n').join('\n$indentation')}'); | ||
return false; | ||
} | ||
if (!javaVersions.every((String element) => element == '$version')) { |
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.
I would strongly prefer making this the first check instead of the second, since if you don't already know there's going to be a check that they all match, the check above that only the first element is the right version seems wrong.
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.
Yeah I can make this change :) standby
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.
Done
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.
It's still second; I would put this before line 465's check.
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.
Still LGTM.
'$indentation${minimumJavaVersionError.split('\n').join('\n$indentation')}'); | ||
return false; | ||
} | ||
if (!javaVersions.every((String element) => element == '$version')) { |
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.
It's still second; I would put this before line 465's check.
@stuartmorgan-g I misunderstood what ordering you wanted changed in. I thought you wanted the alignment and minimum checks to happen before the namespace and other checks. Changed to validate alignment before value and fixed a test setup that failed as a result. |
…va versions and a minimum (17) (flutter/packages#10206)
…va versions and a minimum (17) (flutter/packages#10206)
Hold review/merge until #10201 has merged.
This pr is code review feedback from #10201 (comment)
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.md
to add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///
).