[pigeon] a few tweaks to the output after having upgraded video_player#663
Conversation
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with some minor changes.
packages/pigeon/bin/run_tests.dart
Outdated
| /// usage: pub run pigeon:run_tests | ||
| //////////////////////////////////////////////////////////////////////////////// | ||
| import 'dart:io' show Process, Platform, exit, stdout, stderr; | ||
| import 'dart:io' show Process, Platform, exit, stdout, stderr, File; |
There was a problem hiding this comment.
Nit: how about grouping the classes and variables, rather than just using whatever order they were added in.
There was a problem hiding this comment.
Done, it might be worth putting in a feature request for the dart team.
packages/pigeon/bin/run_tests.dart
Outdated
| // Delete these files that were just generated to help with the analyzer step. | ||
| File('$flutterUnitTestsPath/lib/message.gen.dart').deleteSync(); | ||
| File('$flutterUnitTestsPath/test/message_test.dart').deleteSync(); | ||
|
|
There was a problem hiding this comment.
Please use local variables for these paths instead of repeating the construction twice.
There was a problem hiding this comment.
Also, this whole new block seems like it would be much better as a separate method, so that it's clear that it's a self-contained block that's unrelated to the code above and below (e.g., Future<int> _analyzeGeneratedCode(String path)). That would make this method easier to follow since it would be higher level..
Fixes some pigeon issues that surfaced in flutter/plugins#4726
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.