Conversation
flutter-dashboard
bot
added
the
tool
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). 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. |
|
@GaryQian is it ready for review? |
|
Ready for review! |
There was a problem hiding this comment.
Ignore the commented lines, this will all need to be updated/cleaned up depending on the order these subcommands land. These commented lines won't make it into the codebase.
|
@christopherfujino This one needs review as well. Any comments made in the other PRs that have equivalents here will be synced, so no need to duplicate suggestions |
|
@christopherfujino @blasten @zanderso Friendly ping for review :) |
There was a problem hiding this comment.
nit: I think you can use a multi-line string here to make this line shorter.
There was a problem hiding this comment.
nit: long line here and below.
| final FlutterProject project = projectDirectory == null ? FlutterProject.current() : flutterProjectFactory.fromDirectory(fileSystem.directory(projectDirectory)); | |
| final FlutterProject project = projectDirectory == null | |
| ? FlutterProject.current() | |
| : flutterProjectFactory.fromDirectory(fileSystem.directory(projectDirectory)); |
There was a problem hiding this comment.
I think platform-specific code should be factored out, and the code in this file should be platform-agnostic.
There was a problem hiding this comment.
Refactored into migrate/migrate_update_locks.dart
There was a problem hiding this comment.
nit: line is long. Maybe, each flag could be in its own line, and have a trailing comma at the end
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
| FileSystem fileSystem) async { | |
| FileSystem fileSystem, | |
| ) async { |
There was a problem hiding this comment.
| Terminal terminal) async { | |
| Terminal terminal, | |
| ) async { |
There was a problem hiding this comment.
nit: indentation should match the closing bracket } indentation.
There was a problem hiding this comment.
nit: consider breaking this line https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-a-maximum-line-length-of-80-characters
There was a problem hiding this comment.
why do we need this?
There was a problem hiding this comment.
This command prompts devs on if they want to update their lockfiles after migration is applied.
There was a problem hiding this comment.
Where is this option configured?
There was a problem hiding this comment.
Ahh, this is a remnant of syncing changes between the parallel migrate PRs. I'll fix it!
There was a problem hiding this comment.
can you interpolate workingDirectory in case the problem is that the tool is looking in the wrong place?
There was a problem hiding this comment.
should we print error in this case?
There was a problem hiding this comment.
the hasUncommittedChanges method handles printing the error already.
|
@christopherfujino Comments addressed! |
| prompt: 'Do you want the tool to run `flutter pub upgrade --major-versions`? (y)es, (n)o', | ||
| defaultChoiceIndex: 1, | ||
| ); | ||
| } on StateError catch(e) { |
There was a problem hiding this comment.
Why do we catch StateError here?
There was a problem hiding this comment.
It is thrown if useTerminalUi is false, but since we now set it to true right before, I can just remove this try-catch block
| logger.printStatus('\nDart dependency locking detected in pubspec.yaml.'); | ||
| String selection = 'y'; | ||
| try { | ||
| selection = await terminal.promptForCharInput( |
There was a problem hiding this comment.
should we set terminal.usesTerminalUi = true; right here?
| } | ||
|
|
||
| /// Checks if gradle dependency locking is used and prompts the developer to | ||
| /// remove and back up the gradle dependenc lockfile. |
There was a problem hiding this comment.
| /// remove and back up the gradle dependenc lockfile. | |
| /// remove and back up the gradle dependency lockfile. |
| } | ||
| } | ||
| } | ||
| // Runs `./gradelw tasks`in the project's android directory. |
There was a problem hiding this comment.
| // Runs `./gradelw tasks`in the project's android directory. | |
| // Runs `./gradlew tasks` in the project's android directory. |
5 participants
flutter migrate applysubcommand.This command checks if there are any merge conflicts and copies the files from the migrate_working_directory to the actual project.
Depends on #101937