-
Notifications
You must be signed in to change notification settings - Fork 129
Conversation
fixes #305 |
const appJSON = await client.versions.exportMethod(appId, versionId) | ||
if (appJSON) { | ||
const outputPath = out ? out : '' | ||
await utils.writeOutput(outputPath, appJSON, force) |
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.
Although it is very clean and helps to reduce code duplication having this on the utils file, I think it is easier to understand the command flow by keeping the flags processing in the command. e.g
if (flags.out) {
writeToFile()
} else {
writeToConsole()
}
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.
Moved flag parsing to cmd
packages/luis/src/utils/index.ts
Outdated
|
||
const writeToConsole = (outputContents: string) => { | ||
const output = JSON.stringify(outputContents, null, 2) | ||
process.stdout.write('App successfully exported\n') |
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.
Please keep command responsibilities at the command level. e.g
// Command class
writeToConsole()
this.log(App successfully exported)
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.
refactored
packages/luis/src/utils/index.ts
Outdated
const validatedPath = utils.validatePath(outputLocation, '', force) | ||
await fs.ensureFile(outputLocation) | ||
await fs.writeJson(validatedPath, content, {spaces: 2}) | ||
process.stdout.write(`File successfully written: ${validatedPath}`) |
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.
Please keep command responsibilities(logging) at the command level. e.g
// Command class
const path = writeToFile()
this.log(`File successfully written: ${path}`)
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.
I actually wanted to do that too, but with this model, the tests have a hard time reading the message since 'ctx.stdout' typically only reads the first line. need to think about how to overcome that:
expect(ctx.stdout).to.contain('File successfully written')
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.log uses process.stdout.write under the hood
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.
refactored
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.
CIL
@@ -36,17 +36,27 @@ const getLUISClient = (subscriptionKey: string, endpoint: string) => { | |||
return luisClient | |||
} | |||
|
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.
I added this util function in order to limit the allowed config vars to 'appId, region, subscriptionKey, versionId' per Eyal's specs
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.
Nice work!
* Adding luis:version:export cmd * Remove dependency * Update example * Only store certain values in config, per specs * Refactor / keep logging and flag parsing in cmd only * Refactor write file error handling
Fixes #47 |
No description provided.