Skip to content

fix(code-review): apply #277 review fixes #278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arrrrny merged 1 commit into from
Aug 9, 2026
Merged

fix(code-review): apply #277 review fixes #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/src/cli/cli_runner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,12 @@ USAGE:

BOOTSTRAP:
setup <name> Create a new Flutter/Dart app with zuraffa deps wired in
init Wire zuraffa dependencies + scaffold a test entity
init Alias of initialize — wire deps + scaffold a test entity

CORE COMMANDS:
make <Name> Canonical architecture/code generation command
feature <Name> Wrapper over `make --preset=feature`
initialize Alias of init — wire deps + scaffold a test entity
initialize Wire zuraffa dependencies + scaffold a test entity
entity Create and manage Zorphy entities
config Manage ZFA configuration
doctor Check your environment and v5 migration readiness
Expand Down
25 changes: 18 additions & 7 deletions lib/src/commands/initialize_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'dart:io';
import 'package:args/args.dart';
import 'package:args/command_runner.dart';
import 'package:path/path.dart' as path;
import '../config/zfa_config.dart';
import '../core/dependencies/dependency_wirer.dart';
Expand All @@ -9,8 +10,10 @@ import '../utils/string_utils.dart';
class InitializeCommand {
static const String fixedEntityOutput = ZfaConfig.fixedEntityOutput;

Future<void> execute(List<String> args) async {
final parser = ArgParser()
/// The parser used by [execute]. Exposed so tests exercise the real parser
/// instead of a duplicated copy.
static ArgParser buildParser() {
return ArgParser()
..addOption(
'entity',
abbr: 'e',
Expand Down Expand Up @@ -54,6 +57,10 @@ class InitializeCommand {
negatable: false,
)
..addFlag('help', abbr: 'h', help: 'Show help', negatable: false);
}

Future<void> execute(List<String> args) async {
final parser = buildParser();

final results = parser.parse(args);

Expand All @@ -70,8 +77,10 @@ class InitializeCommand {
final noDeps = results['no-deps'] as bool;

if (depsOnly && noDeps) {
print('❌ --deps-only and --no-deps are mutually exclusive.');
exit(1);
throw UsageException(
'--deps-only and --no-deps are mutually exclusive.',
parser.usage,
);
}

// --- Dependency wiring (issue #275) -----------------------------------
Expand All @@ -82,11 +91,11 @@ class InitializeCommand {
if (!noDeps) {
final pubspecFile = File('pubspec.yaml');
if (!pubspecFile.existsSync()) {
print('❌ No pubspec.yaml found in current directory.');
print(
throw UsageException(
'No pubspec.yaml found in current directory.\n'
' Run `zfa setup <name>` to create a new app, or cd to a project root.',
parser.usage,
);
exit(1);
}

final pubspecContent = pubspecFile.readAsStringSync();
Expand All @@ -106,6 +115,8 @@ class InitializeCommand {
'${wireResult.failed.join(', ')}',
);
print(' Add them manually and re-run `zfa init`.');
// Non-zero exit so CI can distinguish a partial wiring.
throw StateError('Some dependencies could not be wired automatically.');
}

// Ensure build.yaml + domain directory structure exist.
Expand Down
42 changes: 39 additions & 3 deletions lib/src/commands/setup_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class SetupCommand extends Command<void> {
argParser.addOption(
'org',
valueHelp: 'com.example',
help: 'Organization name for `flutter create` (e.g. com.example).',
help: 'Organization name for `flutter create` (e.g. com.example; ignored with --dart).',
);
argParser.addFlag(
'dry-run',
Expand All @@ -57,7 +57,8 @@ class SetupCommand extends Command<void> {
'force',
abbr: 'f',
negatable: false,
help: 'Overwrite the target directory if it already exists.',
help: 'Delete and recreate the target directory if it already exists '
'(requires confirmation on a terminal).',
);
argParser.addFlag(
'verbose',
Expand Down Expand Up @@ -111,6 +112,7 @@ class SetupCommand extends Command<void> {

// 2. Wire the standard zuraffa dependency set (dart pub add + overrides).
print('\n[2/5] Wiring zuraffa dependencies...');
WireResult? wireResult;
if (dryRun) {
final missing = DependencyWirer.findMissing(
_dryRunPubspec(appName, isFlutter),
Expand All @@ -121,7 +123,7 @@ class SetupCommand extends Command<void> {
print(' • $spec');
}
} else {
await DependencyWirer.wire(
wireResult = await DependencyWirer.wire(
isFlutter: isFlutter,
dryRun: false,
projectRoot: appName,
Expand Down Expand Up @@ -153,6 +155,15 @@ class SetupCommand extends Command<void> {

// 5. Summary.
print('\n[5/5] Setup complete!');
if (wireResult != null && !wireResult.isSuccess) {
print(
'\n⚠️ Some dependencies could not be wired automatically: '
'${wireResult.failed.join(', ')}',
);
print(' Add them manually to pubspec.yaml and re-run `zfa init`.');
// Non-zero exit so CI can distinguish a partial bootstrap.
throw StateError('Some dependencies could not be wired automatically.');
}
Comment on lines +158 to +166

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Stop setup immediately after dependency wiring fails.

Steps 3 and 4 run before this check. A failed WireResult therefore still creates project structure and .zfa.json, then prints Setup complete! before throwing. Move this failure check directly after DependencyWirer.wire and before step 3.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/src/commands/setup_command.dart` around lines 158 - 166, Move the
existing wireResult failure check immediately after the DependencyWirer.wire
call and before step 3 begins. Preserve its warning output and StateError
behavior so setup stops before creating project structure or .zfa.json when
wiring fails.


// Next steps.
print('\n── Next steps ──');
Expand Down Expand Up @@ -187,6 +198,17 @@ class SetupCommand extends Command<void> {
return false;
}
if (!dryRun) {
final absolutePath = targetDir.absolute.path;
final entryCount = _countEntries(targetDir);
print(' ⚠️ --force will DELETE: $absolutePath ($entryCount entries)');
if (stdin.hasTerminal) {
stdout.write(' Type "yes" to confirm deletion: ');
final answer = stdin.readLineSync()?.trim().toLowerCase();
if (answer != 'yes') {
print(' Aborted. Target directory was NOT deleted.');
return false;
}
}
await targetDir.delete(recursive: true);
print(' Removed existing $appName (--force)');
} else {
Expand Down Expand Up @@ -230,6 +252,11 @@ class SetupCommand extends Command<void> {
}

// Pure Dart package.
if ((platforms != null && platforms.isNotEmpty) ||
(org != null && org.isNotEmpty)) {
print(' ⚠️ --platforms/--org are ignored with --dart '
'(dart create has no equivalent).');
}
final args = <String>['create', '-t', 'package', appName];
if (dryRun) {
print('\n[1/5] Would run: dart ${args.join(" ")}');
Expand All @@ -250,6 +277,15 @@ class SetupCommand extends Command<void> {
return true;
}

/// Counts all files and directories under [dir] (recursively).
static int _countEntries(Directory dir) {
var count = 0;
for (final _ in dir.listSync(recursive: true)) {
count++;
}
return count;
}

/// Minimal pubspec for dry-run preview (so findMissing has something to parse).
String _dryRunPubspec(String name, bool isFlutter) {
final flutterDep = isFlutter ? '''
Expand Down
53 changes: 36 additions & 17 deletions lib/src/core/dependencies/dependency_wirer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'dart:io';

import 'package:path/path.dart' as path;
import 'package:yaml/yaml.dart';

/// Kind of dependency entry in pubspec.yaml.
Expand Down Expand Up @@ -210,10 +211,8 @@ class DependencyWirer {
if (!result.endsWith('\n\n')) {
result = '$result\n';
}
return '$result dependency_overrides:\n $key: $value\n'.replaceFirst(
' dependency_overrides',
'dependency_overrides',
);
// Header is written at column zero; no leading-space workaround needed.
return '${result}dependency_overrides:\n $key: $value\n';
}

// Walk the existing section looking for the key.
Expand Down Expand Up @@ -247,7 +246,7 @@ class DependencyWirer {
String? projectRoot,
}) async {
final root = projectRoot ?? Directory.current.path;
final pubspecFile = File('$root/pubspec.yaml');
final pubspecFile = File(path.join(root, 'pubspec.yaml'));

if (!pubspecFile.existsSync()) {
print('❌ No pubspec.yaml found in $root');
Expand All @@ -259,10 +258,15 @@ class DependencyWirer {

final content = pubspecFile.readAsStringSync();
final missing = findMissing(content, isFlutter: isFlutter);
// Names already present before wiring — reported as skipped so
// `WireResult.didNothing` is accurate when everything was already wired.
final skippedNames = standardSet(
isFlutter: isFlutter,
).where((s) => !missing.contains(s)).map((s) => s.name).toList();

if (missing.isEmpty) {
print('✅ All zuraffa dependencies are already present.');
return const WireResult();
return WireResult(skipped: skippedNames);
}

print('🔧 Wiring ${missing.length} missing dependenc${missing.length == 1 ? 'y' : 'ies'}:');
Expand All @@ -289,12 +293,15 @@ class DependencyWirer {
.where((s) => s.kind == DependencyKind.override)
.toList();

// --- regular / dev deps via `dart pub add` ---
// --- regular / dev deps via `pub add` ---
// Flutter projects must use `flutter pub add`/`flutter pub get`: the
// standalone `dart` executable cannot resolve `sdk: flutter` deps.
final pubExecutable = isFlutter ? 'flutter' : 'dart';
for (final spec in pubAddSpecs) {
final args = _buildPubAddArgs(spec);
try {
final result = await Process.run(
'dart',
pubExecutable,
['pub', 'add', ...args],
workingDirectory: root,
);
Expand Down Expand Up @@ -322,31 +329,43 @@ class DependencyWirer {
spec.name,
spec.version ?? '',
);
added.add(spec.name);
print(' ✅ Added override:${spec.name}=${spec.version}');
}
await pubspecFile.writeAsString(newContent);
try {
await pubspecFile.writeAsString(newContent);
// Record overrides as added only after the write succeeds.
added.addAll(overrideSpecs.map((s) => s.name));
} catch (e) {
print(
' ⚠️ Failed to write dependency_overrides to pubspec.yaml: $e',
);
failed.addAll(overrideSpecs.map((s) => s.name));
Comment on lines +334 to +342

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Log override success after the write succeeds.

The loop logs ✅ Added override... before writeAsString completes. If the write fails, the output reports the same override as both added and failed. Move the success log after a successful write.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/src/core/dependencies/dependency_wirer.dart` around lines 334 - 342, Move
the `✅ Added override...` success logging in the dependency override loop to
execute only after `pubspecFile.writeAsString(newContent)` completes
successfully. Keep `added.addAll` and the existing failure handling aligned with
the write result so a failed write reports only failure.

}
// Re-resolve so the override takes effect.
try {
final getResult = await Process.run(
'dart',
pubExecutable,
['pub', 'get'],
workingDirectory: root,
);
if (getResult.exitCode != 0) {
final err = getResult.stderr.toString().trim();
if (err.isNotEmpty) {
print(' ⚠️ dart pub get reported issues after override edit:');
print(
' ⚠️ $pubExecutable pub get reported issues after override edit:',
);
print(' ${err.split('\n').take(3).join('\n ')}');
}
}
Comment on lines 351 to 359

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make a failed pub get fail the wiring result.

When pubExecutable pub get returns nonzero, this code only prints diagnostics. WireResult.isSuccess remains true, so InitializeCommand and SetupCommand can continue and exit successfully after an unresolved override. Record a resolution failure in WireResult before returning.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/src/core/dependencies/dependency_wirer.dart` around lines 351 - 359,
Update the dependency-wiring flow handling nonzero `getResult.exitCode` so it
records a resolution failure in `WireResult` before returning, rather than only
printing diagnostics. Ensure `WireResult.isSuccess` becomes false and that
`InitializeCommand` and `SetupCommand` receive the failed result and cannot
continue as successful after an unresolved override.

} catch (e) {
// Non-fatal: overrides are written; user can resolve later.
print(' ⚠️ Could not run dart pub get after override edit: $e');
print(
' ⚠️ Could not run $pubExecutable pub get after override edit: $e',
);
}
}

return WireResult(added: added, failed: failed);
return WireResult(added: added, skipped: skippedNames, failed: failed);
}

/// Builds the argument list for `dart pub add` from a [DependencySpec].
Expand Down Expand Up @@ -414,10 +433,10 @@ targets:
final root = projectRoot ?? Directory.current.path;

// build.yaml
final buildYaml = File('$root/build.yaml');
final buildYaml = File(path.join(root, 'build.yaml'));
if (!buildYaml.existsSync()) {
if (dryRun) {
print(' Would create: $root/build.yaml');
print(' Would create: ${path.join(root, 'build.yaml')}');
} else {
await buildYaml.writeAsString(buildYamlContent);
print(' Created: build.yaml');
Expand All @@ -426,7 +445,7 @@ targets:

// Domain/data directories
for (final dir in standardDirs) {
final full = '$root/$dir';
final full = path.join(root, dir);
if (!Directory(full).existsSync()) {
if (dryRun) {
print(' Would create: $full');
Expand Down
Loading
Loading