-
Notifications
You must be signed in to change notification settings - Fork 348
add commander example, copy and remove file #27
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for your PR!
From my side I'd like to suggest few points:
- Convert code to
standard
code-style as it looks like all others examples are using it - Merge separate actions with files with 1 CLI tool that provides multiple commands. This will eliminate code duplication in different folders
@rodion-arr Thank you for your feedbacks! |
const exist = (dir) => { | ||
try { | ||
fs.accessSync( | ||
dir, | ||
fs.constants.F_OK | fs.constants.R_OK | fs.constants.W_OK | ||
) // tests a user's permissions for the file or directory specified by path | ||
return true // if dir exists, return true. | ||
} catch (e) { | ||
return false // if dir not exists, return false. | ||
} | ||
} |
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'd suggest to move this function into separate module and reuse in both command files
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.
Okay it's good i will change the code. thank you 👍
@rodion-arr Thanks for feedback! |
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.
OK for me, let’s wait for other reviews
Looks good. If you can add unit tests for the |
const path = require('path') | ||
const { exist } = require('./util') | ||
|
||
const rimraf = (p) => { |
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.
Variable names such as path
, directory
would be more readable than p
, d
"lint": "standard", | ||
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, | ||
"author": "", |
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.
Include Author Name
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, | ||
"author": "", | ||
"license": "ISC", |
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.
Use "MIT" License please
"main": "index.js", | ||
"scripts": { | ||
"lint": "standard", | ||
"test": "echo \"Error: no test specified\" && exit 1" |
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 add some unit tests
Add new CLI example using commander module. I added copy and remove file using fs, path and commander module