-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[flutter_migrate] Add ios-language version error handling from flutter 3.35 #9812
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
base: main
Are you sure you want to change the base?
[flutter_migrate] Add ios-language version error handling from flutter 3.35 #9812
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. |
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.
Code Review
This pull request updates flutter_migrate
to handle an error from newer Flutter versions where the ios-language
option is no longer supported for application templates. The changes modify createFromTemplates
to make iosLanguage
nullable, conditionally add the command-line argument, and recursively retry without it if the specific error is encountered. The package version and changelog have been updated to reflect these changes. My review identifies a critical issue in the implementation of the recursion limit that could lead to infinite loops, and also points out the lack of tests for the new logic.
// Catch errors due to parameters not supported anymore for apps in flutter 3.35 | ||
if (error.contains( | ||
'The "ios-language" option is only supported for "--template=plugin".')) { | ||
return createFromTemplates( | ||
flutterBinPath, | ||
name: name, | ||
legacyNameParameter: legacyNameParameter, | ||
androidLanguage: androidLanguage, | ||
iosLanguage: null, | ||
outputDirectory: outputDirectory, | ||
platforms: platforms, | ||
iterationsAllowed: iterationsAllowed--, | ||
); | ||
} |
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.
This new error handling logic is not covered by any tests. Please consider adding a unit test that mocks ProcessManager
to verify that a recursive call with iosLanguage: null
is made when this specific error occurs. This will help ensure the logic remains correct in the future.1
Style Guide References
Footnotes
Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the state of the PR as posted reflects the checklist please feel free to mark it as ready for review. |
9a0b103
to
a063919
Compare
Thanks for your reply. I have answered a few boxes.
Let me know what you expect from me. Thank you |
https://github.com/flutter/packages?tab=readme-ov-file#issues
It's not clear how that happened; it appears that at some point someone manually published a version with local modifications, which should not have happened. Those steps can be ignored for now though, as the package is currently marked
Lack of comprehensive tests for existing code doesn't exempt changes from testing; it means that the change has highlighted a testing gap, which should be addressed. See the docs for exemption reasons. |
Since flutter 3.35, flutter_migrate throws an exception because ios-language is being passed as argument and is no longer supported.
List which issues are fixed by this PR. You must list at least one issue.
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].On pub.dev, the published version is already 0.1.0 : https://pub.dev/packages/flutter_migrate
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].///
).There's no test for the other 3 error handling in this method, and there's no ProcessManagerMock to return the desired error from the process.