[flutter_plugin_tools] Fix build-examples for packages #4248
Conversation
| if (buildPlatforms.isEmpty) { | ||
| final String unsupported = requestedPlatforms.length == 1 | ||
| ? '${requestedPlatforms.first.label} is not supported' | ||
| : 'None of [${requestedPlatforms.map((_PlatformDetails p) => p.label).join(',')}] are supported'; | ||
| : 'None of [${requestedPlatforms.map((_PlatformDetails p) => p.label).join(', ')}] are supported'; |
There was a problem hiding this comment.
nit: I ran into this too, it probably would be better to pull the join argument to a const or wrap that behavior in a function so it could be updated in one place.
There was a problem hiding this comment.
I pulled out a local function for the two lines that were doing the same display list transform.
| .childDirectory(platform.flutterPlatformDirectory) | ||
| .existsSync()) { | ||
| print('Skipping ${platform.label} for $packageName; not supported.'); | ||
| continue; |
There was a problem hiding this comment.
nit: Instead of using continue do you think an else clause would be preferable?
There was a problem hiding this comment.
I generally don't like to nest non-trivial primary logic in an else clause.
There was a problem hiding this comment.
SGTM, not my preference but not in violation of our style. Academics have argued about it for decades: https://en.wikipedia.org/wiki/Structured_programming#Early_exit
| return pluginSection != null; | ||
| } on FileSystemException { | ||
| return false; | ||
| } on YamlException { |
There was a problem hiding this comment.
Wouldn't it be better to use explicit checks instead of relying on exceptions? It seems like a malformed yaml file would result in a isFlutterPlugin == false instead of crashing the tool which I would have expected.
There was a problem hiding this comment.
Wouldn't it be better to use explicit checks instead of relying on exceptions?
Removed the FileSystemException in favor of an explicit check. (I'm not sure why the code was originally written that way; this was just a move refactor).
It seems like a malformed yaml file would result in a
isFlutterPlugin == falseinstead of crashing the tool which I would have expected.
In practice that wouldn't actually be a problem since we have a different CI test that validates the pubspecs, but I agree we shouldn't rely on it (and in fact, that's much more recent than this pubspec parsing code). I removed the exception handler for YamlException, and I've added a catch-all to PackageLoopingCommand's runForPackage. That covers most of the code the tool runs at this point, and is a good idea to add in general since a central goal of that command base class is to always keep going, and then report all errors at the end.
| .childDirectory(platform.flutterPlatformDirectory) | ||
| .existsSync()) { | ||
| print('Skipping ${platform.label} for $packageName; not supported.'); | ||
| continue; |
There was a problem hiding this comment.
SGTM, not my preference but not in violation of our style. Academics have argued about it for decades: https://en.wikipedia.org/wiki/Structured_programming#Early_exit
The build-examples command was filtering what it attempted to build by plugin platform, which means it never does anything for non-plugin packages. flutter/packages has steps that run this command, which suggests it used to work and regressed at some point, but nobody noticed; this will re-enable those builds so that we are getting CI coverage that the examples in flutter/packages build. Mostly fixes flutter/flutter#88435 (needs a flutter/packages tool pin roll to pick this up)
The build-examples command was filtering what it attempted to build by plugin platform, which means it never does anything for non-plugin packages. flutter/packages has steps that run this command, which suggests it used to work and regressed at some point, but nobody noticed; this will re-enable those builds so that we are getting CI coverage that the examples in flutter/packages build. Mostly fixes flutter/flutter#88435 (needs a flutter/packages tool pin roll to pick this up)
The build-examples command was filtering what it attempted to build by plugin platform, which means it never does anything for non-plugin packages. flutter/packages has steps that run this command, which suggests it used to work and regressed at some point, but nobody noticed; this will re-enable those builds so that we are getting CI coverage that the examples in flutter/packages build.
Mostly fixes flutter/flutter#88435 (needs a flutter/packages tool pin roll to pick this up)
Pre-launch Checklist
dart format.)[shared_preferences]///).