[plugin_tools] build-examples .pluginToolsConfig.yaml support #4305
Conversation
| @@ -16,7 +17,10 @@ import 'common/repository_package.dart'; | |||
| /// Key for APK. | |||
| const String _platformFlagApk = 'apk'; | |||
|
|
|||
| const String _pluginToolsConfigFileName = '.pluginToolsConfig.yaml'; | |||
There was a problem hiding this comment.
This is a super-generic name. If we're going to use this, I think we should make the YAML format not just a flat list so that we can add other tool configuration options there in the future (which I think is actually fairly likely) without breaking everything. E.g.:
buildFlags:
global:
- foo
- bar
That would let us add both arbitrary other tool config options, and also other build flags (like platform specific flags) later without a format break.
There was a problem hiding this comment.
I don't think we can know what format to pick to avoid a format break, but ok. (For example, what if we discover that actually what we want is for this to be a list of buildFlags, so that you can try running with several configurations? So the root should be a list rather than a map? Or, maybe the root should be platforms, with buildFlags as the second level. Or...)
There was a problem hiding this comment.
But when your root is a map you can always change things without it being a hard breaking change, because you can just use a different root key for the new format. Then eventually you can remove support for the old key.
(In this case the cost of the breaking change would be pretty low since we only guarantee that the tool works for two repos that we control; maybe I'm over-engineering based on experiences with things where it's much harder when you can't do the incremental change via a map.)
There was a problem hiding this comment.
i think things are a lot less clear-cut in practice than this (e.g. changing from a list to a map is non-breaking too since you can just keep on supporting the old list mode, and sometimes a map's keyspace is complete, e.g. the semantics are that you do something for each key, in which case the old key would conflict with the new keyspace, etc), but it's not a big deal either way.
|
Ok, updated as requested. |
| ]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Optional nit: with all the error checking this is long enough that I would extract it to a List<String> _getPackageSpecificBuildFlags(RepositoryPackage package) directory.
There was a problem hiding this comment.
ok, extracted to function
| printError( | ||
| 'That list must contain a list of arguments to pass to the flutter tool.'); | ||
| printError( | ||
| 'For example, the $_pluginToolsConfigFileName file could look like:'); |
There was a problem hiding this comment.
Optional nit: are the slight variations of error messages actually buying us anything here over just having one constant (like the example) for any invalid structure, that describes the correct structure? It doesn't seem like someone is going to have a hard time figuring out which part they've done wrong from a catch-all message and example.
There was a problem hiding this comment.
in my experience, more detail in error messages (if it's accurate) is always more helpful. :-)
| @@ -379,7 +379,6 @@ void main() { | |||
| ]), | |||
| ); | |||
|
|
|||
| print(processRunner.recordedCalls); | |||
There was a problem hiding this comment.
Thanks, I thought I'd caught all of these during all the refactoring/cleanup work. Apparently it spread to more places than I'd noticed.
There was a problem hiding this comment.
there's still some prints somewhere in the tests but i couldn't track them down in the 10 seconds i spent trying. It says "Determine diff with base sha:" sometimes, and "null" once.
There was a problem hiding this comment.
"Determine diff with base sha:" sometimes
I bet that's from a test of the helper directly, not going through a command runner that captures output. That helper really shouldn't log anyway, I just haven't gotten around to fixing it.
Hixie
added
the
waiting for tree to go green
Pre-launch Checklist
dart format.)[shared_preferences]///).