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. |
There was a problem hiding this comment.
should it mention the default directory?
There was a problem hiding this comment.
Yes, ill add it in the help text!
There was a problem hiding this comment.
Idk if this will be painful, but can we name this something else, maybe workspace-directory? I think this will be confusing with the current working directory of the tool invocation. In reading the code, I thought this field was a way to override the project directory.
There was a problem hiding this comment.
Renamed to migrate_staging_dir and the parameter to staging-directory
There was a problem hiding this comment.
Many of these flags assist in spam-reduction to make it easier to find the diffs/info the user is actually looking for.
There was a problem hiding this comment.
what is the use case for disabling this flag?
There was a problem hiding this comment.
The diffs can often be quite spammy, programmatic uses especially may not wish to have such an extensive output.
There was a problem hiding this comment.
for simplicity, should it only assume the current directory is the project directory?
There was a problem hiding this comment.
This tool is also intended to be used programmatically, where this option helps immensely.
There was a problem hiding this comment.
It looks like if this is not provided we default to the current working directory, can we document this?
There was a problem hiding this comment.
These lines just include the file names being diffed for example:
diff --git a/Users/garyq/projects/vanillabase1226/android/app/src/main/res/values/styles.xml b/Users/garyq/projects/vanillabase1226/migrate_working_dir/android/app/src/main/res/values/styles.xml
index 1f83a33..8dbda4a 100644
There was a problem hiding this comment.
Can you flesh out the dartdoc with this example?
There was a problem hiding this comment.
what happens if a line starts with ---, +++, && in the original file?
There was a problem hiding this comment.
The first character of the line is reserved for the + and - character, so lines that start with those strings in the original files would appear as _--- _+++ and _&& where underscore is a space. The initial space character would cause the startsWith check to fail.
There was a problem hiding this comment.
Can you flesh out the dartdoc with this example?
There was a problem hiding this comment.
Idk if this will be painful, but can we name this something else, maybe workspace-directory? I think this will be confusing with the current working directory of the tool invocation. In reading the code, I thought this field was a way to override the project directory.
There was a problem hiding this comment.
It looks like if this is not provided we default to the current working directory, can we document this?
There was a problem hiding this comment.
| // import 'package:flutter_tools/src/base/terminal.dart'; |
There was a problem hiding this comment.
can you move this file to packages/flutter_tools/test/command.shard/permeable/migrate_status_test.dart?
There was a problem hiding this comment.
is this needed? we explicitly created it on line 55, are we testing it didn't get deleted?
There was a problem hiding this comment.
Ahh, this is leftover from another test this was based on. I can remove.
|
Ready for another round. Thanks! @christopherfujino |
There was a problem hiding this comment.
You can get a path context from the filesystem and use https://pub.dev/documentation/path/latest/path/isAbsolute.html instead of re-implementing this.
There was a problem hiding this comment.
Yep, already did it, just needed to sync it with the other PRs
5 participants
flutter migrate statussubcommand.This command shows the diffs staged in the migrate working directory and the manifest of files to be changed.
Depends on #101937