Add reporting for copy action #86
Conversation
src/cli/commandModules/copy.ts
Outdated
| @@ -17,7 +17,26 @@ const commandModule: CommandModule< | |||
| return withIgnoreOption(withStateOption(withDestinationOption(yargs))); | |||
| }, | |||
| handler: async (args: Arguments<CopyArgs>) => { | |||
| const errors = await copy(args); | |||
| const { errors, filesCopied } = await copy(args); | |||
| filesCopied.forEach(({ fromPath, toPath, type, errorMessage }) => { | |||
There was a problem hiding this comment.
nit: Might be nice to break this into a separate function/interface like report() down the line - this adds some heft that isn't clearly "format and report the output" until you read through it
src/bluehawk/actions/copy.ts
Outdated
| @@ -100,6 +120,11 @@ export const copy = async (args: CopyArgs): Promise<string[]> => { | |||
| ); | |||
| const targetPath = path.join(directory, document.basename); | |||
|
|
|||
| const copyInfo: CopyInfo = { | |||
There was a problem hiding this comment.
| const copyInfo: CopyInfo = { | |
| const copyInfo = { |
nit: CopyResult.filesCopied is already a CopyInfo[] so it should be type safe to just push this later on. The explicit type doesn't hurt for now though so feel free to leave it in.
There was a problem hiding this comment.
Usually I prefer less explicit typing, but in this case the explicit type ensures the instantiation (this line to ~127) matches the type. If we remove the typing and the type changes, then the error will appear when you push rather than instantiate. Not a huge deal, but that might be a head scratcher for someone.
|
A few improvements we might make:
e.g. |
src/bluehawk/bluehawk.ts
Outdated
| @@ -24,6 +26,7 @@ interface BluehawkConfiguration { | |||
| } | |||
|
|
|||
| type ParseAndProcessOptions = ProcessOptions & { | |||
| reporter?: ActionReporter; | |||
There was a problem hiding this comment.
I think this is the only place where reporter is optional - was that intentional?
| } | ||
| onFileParsed(event: FileParsedEvent): void { | ||
| ++this._count.textFiles; | ||
| console.log(`parsed file: ${event.sourcePath}`); |
There was a problem hiding this comment.
Always logging a line for each event would probably mean very verbose logs, especially if we run commands on a directory.
In most cases I think it would suffice to see only the output of summary().
Any thoughts on a configurable reporter? Maybe we could configure it to log specific events or set a general verbosity level.
"Copy" action reports how many files were copied to console.
Not super happy with this design, as I think a more flexible reporting system could work better (and apply to more actions than just copy), but this is here for discussion/as a first step.
Example: