Complete refactoring #2
Conversation
package.json
Outdated
| ] | ||
| ], | ||
| "dependencies": { | ||
| "rimraf": "^3.0.0" |
| @@ -3,15 +3,17 @@ import { prompt } from 'inquirer' | |||
| import * as colors from 'colors' | |||
|
|
|||
| const fs = require('fs') | |||
There was a problem hiding this comment.
Use fs-extra for all file system operations and rename next line from fsExtra to fs - every method from fs is supported by fs-extra, so you can use it as a drop-in replacement.
| @@ -113,7 +116,12 @@ Creating... | |||
| spinner.succeed() | |||
| await this.botonicApiService.buildIfChanged(false) | |||
| this.botonicApiService.beforeExit() | |||
| await exec('mv ../.botonic.json .') | |||
| try { | |||
| await fsExtra.move('../.botonic.json', './botonic.json') | |||
There was a problem hiding this comment.
Not sure if it's a typo or not, it's .botonic.json to botonic.json without dot at the beginning but in the original it just moves the file one directory up - is that what we want?
|
|
||
| import { BotonicAPIService } from '../botonicAPIService' | ||
| import { track } from '../utils' | ||
|
|
||
| const util = require('util') | ||
| const ora = require('ora') | ||
| const exec = util.promisify(require('child_process').exec) | ||
| const fsExtra = require('fs-extra') |
| @@ -3,15 +3,17 @@ import { prompt } from 'inquirer' | |||
| import * as colors from 'colors' | |||
|
|
|||
| const fs = require('fs') | |||
| const fsExtra = require('fs-extra') | |||
There was a problem hiding this comment.
Not really related to this line but rather whole block - notice that there are import statements above it, but below that there are requires.... I think it should be consistent (you can use imports everywhere - it's TypeScript), but there are a lot of other code quality issues that linter would report so I'm not sure if you should be handling that or not.
| @@ -62,13 +64,13 @@ Uploading... | |||
| await this.botonicApiService.getMoreBots(bots, nextBots) | |||
| } | |||
| let bot = bots.filter(b => b.name === botName)[0] | |||
| if (bot == undefined) { | |||
| if (bot === undefined) { | |||
There was a problem hiding this comment.
This looks really fishy to be honest... Generally bot should always return either some kind of "bot" object or undefined but you don't really check undefined like that ever. And here we actually only want this bot object. I'd suggest switching contents of if and else blocks and use if (bot).
| } | ||
| ]).then((inp: any) => { | ||
| if (inp.signupConfirmation == choices[1]) return this.askLogin() | ||
| if (inp.signupConfirmation === choices[1]) return this.askLogin() |
There was a problem hiding this comment.
Personal preference but I don't really like inline if/else, you could replace this whole block with ternary.
| @@ -131,7 +134,7 @@ Uploading... | |||
| // Show the current bot in credentials at top of the list | |||
| let first_id = this.botonicApiService.bot.id | |||
There was a problem hiding this comment.
Why is everything in camelCase (or so I thought) and this thing is in snake_case? Again not sure if you should even care about this but yeah...
| @@ -131,7 +134,7 @@ Uploading... | |||
| // Show the current bot in credentials at top of the list | |||
| let first_id = this.botonicApiService.bot.id | |||
| bots.sort(function(x, y) { | |||
|
Looks good to me |
No description provided.