Skip to content

Commit dfe7f84

Browse files
[Conductor] Add documentation to individual files (#121277)
* add documentation to files * Update dev/conductor/core/lib/src/stdio.dart Co-authored-by: Christopher Fujino <[email protected]> * Update dev/conductor/core/lib/src/start.dart Co-authored-by: Christopher Fujino <[email protected]> * Update dev/conductor/core/lib/src/repository.dart Co-authored-by: Christopher Fujino <[email protected]> * Update dev/conductor/core/lib/src/next.dart Co-authored-by: Christopher Fujino <[email protected]> * Update dev/conductor/core/lib/src/git.dart Co-authored-by: Christopher Fujino <[email protected]> * Update dev/conductor/core/lib/src/git.dart Co-authored-by: Christopher Fujino <[email protected]> * Update stdio.dart --------- Co-authored-by: Christopher Fujino <[email protected]>
1 parent 219ff64 commit dfe7f84

File tree

6 files changed

+75
-0
lines changed

6 files changed

+75
-0
lines changed

dev/conductor/core/lib/src/git.dart

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@ import 'package:process/process.dart';
99
import './globals.dart';
1010

1111
/// A wrapper around git process calls that can be mocked for unit testing.
12+
///
13+
/// The `Git` class is a relatively (compared to `Repository`) lightweight
14+
/// abstraction over invocations to the `git` cli tool. The main
15+
/// motivation for creating this class was so that it could be overridden in
16+
/// tests. However, now that tests rely on the [FakeProcessManager] this
17+
/// abstraction is redundant.
1218
class Git {
1319
const Git(this.processManager);
1420

dev/conductor/core/lib/src/next.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ const String kStateOption = 'state-file';
1818
const String kYesFlag = 'yes';
1919

2020
/// Command to proceed from one [pb.ReleasePhase] to the next.
21+
///
22+
/// After `conductor start`, the rest of the release steps are initiated by the
23+
/// user via `conductor next`. Thus this command's behavior is conditional upon
24+
/// which phase of the release the user is currently in. This is implemented
25+
/// with a switch case statement.
2126
class NextCommand extends Command<void> {
2227
NextCommand({
2328
required this.checkouts,

dev/conductor/core/lib/src/repository.dart

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,39 @@ class Remote {
6060
}
6161

6262
/// A source code repository.
63+
///
64+
/// This class is an abstraction over a git
65+
/// repository on the local disk. Ideally this abstraction would hide from
66+
/// the outside libraries what git calls were needed to either read or update
67+
/// data in the underlying repository. In practice, most of the bugs in the
68+
/// conductor codebase are related to the git calls made from this and its
69+
/// subclasses.
70+
///
71+
/// Two factors that make this code more complicated than it would otherwise
72+
/// need to be are:
73+
/// 1. That any particular invocation of the conductor may or may not already
74+
/// have the git checkout present on disk, depending on what commands were
75+
/// previously run; and
76+
/// 2. The need to provide overrides for integration tests (in particular
77+
/// the ability to mark a [Repository] instance as a [localUpstream] made
78+
/// integration tests more hermetic, at the cost of complexity in the
79+
/// implementation).
80+
///
81+
/// The only way to simplify the first factor would be to change the behavior of
82+
/// the conductor tool to be a long-lived dart process that keeps all of its
83+
/// state in memory and blocks on user input. This would add the constraint that
84+
/// the user would need to keep the process running for the duration of a
85+
/// release, which could potentially take multiple days and users could not
86+
/// manually change the state of the release process (via editing the JSON
87+
/// config file). However, these may be reasonable trade-offs to make the
88+
/// codebase simpler and easier to reason about.
89+
///
90+
/// The way to simplify the second factor would be to not put any special
91+
/// handling in this library for integration tests. This would make integration
92+
/// tests more difficult/less hermetic, but the production code more reliable.
93+
/// This is probably the right trade-off to make, as the integration tests were
94+
/// still not hermetic or reliable, and the main integration test was ultimately
95+
/// deleted in #84354.
6396
abstract class Repository {
6497
Repository({
6598
required this.name,

dev/conductor/core/lib/src/start.dart

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,16 @@ const String kVersionOverrideOption = 'version-override';
3434
const String kGithubUsernameOption = 'github-username';
3535

3636
/// Command to print the status of the current Flutter release.
37+
///
38+
/// This command has many required options which the user must provide
39+
/// via command line arguments (or optionally environment variables).
40+
///
41+
/// This command is the one with the worst user experience (as the user has to
42+
/// carefully type out many different options into their terminal) and the one
43+
/// that would benefit the most from a GUI frontend. This command will
44+
/// optionally read its options from an environment variable to facilitate a workflow
45+
/// in which configuration is provided by editing a bash script that sets environment
46+
/// variables and then invokes the conductor tool.
3747
class StartCommand extends Command<void> {
3848
StartCommand({
3949
required this.checkouts,

dev/conductor/core/lib/src/state.dart

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,19 @@ const String stablePostReleaseMsg = """
3333
'\t 4. Post announcement flutter release hotline chat room',
3434
'\t\t Chatroom: ${globals.flutterReleaseHotline}',
3535
""";
36+
// The helper functions in `state.dart` wrap the code-generated dart files in
37+
// `lib/src/proto/`. The most interesting of these functions is:
38+
39+
// * `pb.ConductorState readStateFromFile(File)` - uses the code generated
40+
// `.mergeFromProto3Json()` method to deserialize the JSON content from the
41+
// config file into a Dart instance of the `ConductorState` class.
42+
// * `void writeStateFromFile(File, pb.ConductorState, List<String>)`
43+
// - similarly calls the `.toProto3Json()` method to serialize a
44+
// * `ConductorState` instance to a JSON string which is then written to disk.
45+
// `String phaseInstructions(pb.ConductorState state)` - returns instructions
46+
// for what the user is supposed to do next based on `state.currentPhase`.
47+
// * `String presentState(pb.ConductorState state)` - pretty print the state file.
48+
// This is a little easier to read than the raw JSON.
3649

3750
String luciConsoleLink(String channel, String groupName) {
3851
assert(

dev/conductor/core/lib/src/stdio.dart

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@ import 'dart:io' as io;
66

77
import 'package:meta/meta.dart';
88

9+
/// An interface for presenting text output to the user.
10+
///
11+
/// Although this could have been simplified by calling `print()`
12+
/// from the tool, this abstraction allows unit tests to verify output
13+
/// and allows a GUI frontend to provide an alternative implementation.
14+
///
15+
/// User input probably should be part of this class–however it is currently
16+
/// part of context.dart.
917
abstract class Stdio {
1018
final List<String> logs = <String>[];
1119

0 commit comments

Comments
 (0)