-
Notifications
You must be signed in to change notification settings - Fork 7
feat(cli): Use @ocap/utils logger #485
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.
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
Conversation
eb9c24c to
8b0ad6f
Compare
rekmarks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of inline items, and also:
The name of this PR should be feat(cli): Use @ocap/utils logger. If we were only adding tests the current name would be fine, but we're modifying the behavior of the module.
| export async function bundleSource( | ||
| logger: Logger, | ||
| target: string, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto re: logger param position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/cli/src/file.test.ts
Outdated
| // Note: the behavior of fileExists is negotiable. The current implementation | ||
| // is a guess at a platform-independent behavior. This test suite documents | ||
| // the behavior of the current implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be moved to the implementation of the function, perhaps phrased along the lines of "may not work on Windows".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rekmarks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Refs: #217
@ocap/utilslogger