Conversation
MtMath
commented
Dec 27, 2025
MtMath
commented
- feat: Introduce Result<T, E> type and UnityError hierarchy
- feat: Export error utilities from package entrypoint
- refactor: Update command executor to return Result and unify error handling
- refactor: UnityEditor methods to use Result pattern
- refactor: UnityHub to use Result pattern and unify error handling
- docs: Update README with Result pattern, error handling, and migration guide
Replaces thrown exceptions with explicit return values using `Result<T, E>`, ensuring consistency in the error flow between services and layers. BREAKING CHANGE: Functions that previously threw exceptions now return `Result<T, E>`.
MtMath
added
documentation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive Result pattern for error handling, replacing exception-based error handling with a type-safe Result<T, E> approach. This change makes error handling explicit, provides rich error context through a new UnityError hierarchy, and enables composable error handling operations.
Key changes:
- Introduces Result<T, E> type with Success and Failure variants, along with utility functions (ok, err, isOk, isErr, map, andThen, etc.)
- Implements UnityError base class with specialized error types (UnityCommandError, UnityHubNotFoundError, UnityEditorNotFoundError, UnityTestError, etc.)
- Refactors all UnityHub and UnityEditor methods to return Result types instead of throwing exceptions or returning boolean/nullable values
- Updates README with comprehensive error handling documentation, migration guide, and updated examples
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/errors/Result.ts | Implements Result<T, E> type and utility functions for functional error handling |
| src/errors/UnityError.ts | Defines UnityError base class and specialized error types with error codes and context |
| src/errors/index.ts | Exports Result and UnityError utilities from errors module |
| src/index.ts | Exports error handling utilities from package entrypoint |
| src/utils/commandExecutor.ts | Refactors executeCommand to return Result<CommandOutput, UnityCommandError> instead of CommandResult |
| src/unityHub.ts | Refactors all UnityHub methods to return Result types with proper error handling |
| src/unityEditor.ts | Refactors all UnityEditor methods to return Result types with proper error handling |
| README.md | Adds comprehensive error handling documentation, examples, migration guide, and error code reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (result.value.stderr) { | ||
| return err( | ||
| new UnityCommandError( | ||
| `Error setting install path: ${result.value.stderr}`, | ||
| result.value.stdout, | ||
| result.value.stderr, | ||
| result.value.exitCode | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Both getInstallPath and setInstallPath treat any stderr output as an error by creating a UnityCommandError. However, some commands may write non-error information to stderr (like warnings or informational messages). This could cause valid operations to be treated as failures. Consider checking the exit code or looking for specific error patterns in stderr before treating it as a failure condition.
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, result.error); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, error); |
There was a problem hiding this comment.
The addModule method returns success immediately with the installer event emitter, but command errors are only logged to console. This means the caller has no way to know if the command execution failed. The method should either include command execution errors in the Result type union, or provide a way for callers to detect execution failures through the event emitter.
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, result.error); | |
| } | |
| }) | |
| .catch((error) => { | |
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, error); | |
| const error = | |
| result.error instanceof Error | |
| ? result.error | |
| : new UnityCommandError( | |
| `Failed to add module(s) ${modules.join(", ")} to Unity ${editorVersion}`, | |
| { editorVersion, modules, cause: result.error } | |
| ); | |
| installerEmitter.Error(error); | |
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, error); | |
| } | |
| }) | |
| .catch((error) => { | |
| const normalizedError = | |
| error instanceof Error | |
| ? error | |
| : new UnityCommandError( | |
| `Failed to add module(s) ${modules.join(", ")} to Unity ${editorVersion}`, | |
| { editorVersion, modules, cause: error } | |
| ); | |
| installerEmitter.Error(normalizedError); | |
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, normalizedError); |
| if (result.value.stderr) { | ||
| console.error(`Error executing method: ${result.value.stderr}`); | ||
| return err( | ||
| new UnityCommandError( | ||
| `Error executing method ${method}: ${result.value.stderr}`, | ||
| result.value.stdout, | ||
| result.value.stderr, | ||
| result.value.exitCode, | ||
| { method, projectInfo } | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The executeMethod function treats any stderr output as an error and returns a UnityCommandError. However, Unity often writes informational logs or warnings to stderr that are not actual errors. This approach may incorrectly classify successful method executions as failures. Consider checking exit codes or looking for specific error patterns in stderr instead of treating any stderr output as a failure.
| * Returns the first successful result or the last error | ||
| * @param results - Array of results | ||
| * @returns The first successful result or the last error | ||
| * @throws If the results array is empty | ||
| */ | ||
| export function firstOk<T, E extends UnityError>(results: Result<T, E>[]): Result<T, E> { | ||
| if (results.length === 0) { | ||
| throw new Error("Cannot call firstOk on empty array"); |
There was a problem hiding this comment.
The firstOk function throws a plain Error instead of returning a Result or using a UnityError. This is inconsistent with the Result pattern philosophy where errors should be explicit in the return type. Consider either returning a Result type or documenting that callers must ensure the array is non-empty.
| * Returns the first successful result or the last error | |
| * @param results - Array of results | |
| * @returns The first successful result or the last error | |
| * @throws If the results array is empty | |
| */ | |
| export function firstOk<T, E extends UnityError>(results: Result<T, E>[]): Result<T, E> { | |
| if (results.length === 0) { | |
| throw new Error("Cannot call firstOk on empty array"); | |
| * Error thrown when `firstOk` is called with an empty array. | |
| */ | |
| class FirstOkEmptyArrayError extends UnityError { | |
| constructor() { | |
| super("Cannot call firstOk on empty array"); | |
| this.name = "FirstOkEmptyArrayError"; | |
| } | |
| } | |
| /** | |
| * Returns the first successful result or the last error | |
| * @param results - Array of results | |
| * @returns The first successful result or the last error | |
| * @throws {FirstOkEmptyArrayError} If the results array is empty | |
| */ | |
| export function firstOk<T, E extends UnityError>(results: Result<T, E>[]): Result<T, E> { | |
| if (results.length === 0) { | |
| throw new FirstOkEmptyArrayError(); |
| const firstVersion = andThen(result, async (installations) => { | ||
| const versions = Object.keys(installations); | ||
| if (versions.length === 0) { | ||
| return err(new UnityInstallationError("No installations found")); |
There was a problem hiding this comment.
The example imports err and UnityInstallationError but these are not included in the import statement. The import statement should include these to make the example complete and functional.
| if (result.value.stderr) { | ||
| return err( | ||
| new UnityCommandError( | ||
| `Error getting install path: ${result.value.stderr}`, | ||
| result.value.stdout, | ||
| result.value.stderr, | ||
| result.value.exitCode | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Both getInstallPath and setInstallPath treat any stderr output as an error by creating a UnityCommandError. However, some commands may write non-error information to stderr (like warnings or informational messages). This could cause valid operations to be treated as failures. Consider checking the exit code or looking for specific error patterns in stderr before treating it as a failure condition.
| this.execUnityHubCommand(args, { | ||
| reject: false, | ||
| //onStderr: (data: string) => installerEmitter.Error(data), | ||
| onStdout: (data: string) => installerEmitter.Progress(data), | ||
| }).catch((error) => { | ||
| console.error(`Error installing Unity ${version}:`, error); | ||
| }); | ||
|
|
||
| return installerEmitter; | ||
| }) | ||
| .then((result) => { | ||
| if (!result.success) { | ||
| console.error(`Error installing Unity ${version}:`, result.error); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.error(`Error installing Unity ${version}:`, error); | ||
| }); | ||
|
|
||
| return ok(installerEmitter); |
There was a problem hiding this comment.
The addEditor method returns success immediately with the installer event emitter, but command errors are only logged to console. This means the caller has no way to know if the command execution failed. The method should either include command execution errors in the Result type union, or provide a way for callers to detect execution failures through the event emitter.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, error); | ||
| }); | ||
| return installerEmitter; | ||
| } catch (error) { | ||
| console.error(`Error adding module ${modules} to Unity ${editorVersion}:`, error); | ||
| throw error; | ||
| } | ||
|
|
||
| return ok(installerEmitter); |
There was a problem hiding this comment.
The result-based API now advertises errors, but addModule always returns ok(installerEmitter) even if execUnityHubCommand fails (e.g., Unity Hub isn’t installed or the CLI rejects the arguments). Because the command is fire-and-forget and the return value is hard-coded to success, callers following the README’s isOk checks will believe the install started while installerEmitter.completed will never resolve. This silently masks UnityHubNotFoundError/UnityCommandError cases instead of propagating them.
Useful? React with 👍 / 👎.
| .catch((error) => { | ||
| console.error(`Error installing Unity ${version}:`, error); | ||
| }); | ||
|
|
||
| return ok(installerEmitter); |
There was a problem hiding this comment.
Similar to addModule, addEditor launches execUnityHubCommand but immediately returns ok(installerEmitter) without checking the result. If Unity Hub is unavailable or the install command errors out, the function still reports success and the emitter will never emit completion, so callers will think an installation started when it actually failed. The new Result contract therefore never surfaces UnityHubNotFoundError/UnityCommandError for failed installs.
Useful? React with 👍 / 👎.
Updates error message for clarity by capitalizing "Unity" in the "No unity installations found" error. Improves user readability and consistency in error handling.
1 participant
