Skip to content
This repository has been archived by the owner on Jun 22, 2024. It is now read-only.

add commander example, copy and remove file #27

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hyeongjun231
Copy link

Add new CLI example using commander module. I added copy and remove file using fs, path and commander module

Copy link
Contributor

@rodion-arr rodion-arr left a 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

cli/commander/removeFile/README.md Outdated Show resolved Hide resolved
@hyeongjun231
Copy link
Author

@rodion-arr Thank you for your feedbacks!
I made a new commit to reflect the changes. Please check.
Thank you

Comment on lines 4 to 14
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.
}
}
Copy link
Contributor

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

Copy link
Author

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 👍

@hyeongjun231
Copy link
Author

@rodion-arr Thanks for feedback!
Could you check this change?
Thank you

Copy link
Contributor

@rodion-arr rodion-arr left a 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

@Thiruppathi
Copy link
Contributor

Looks good. If you can add unit tests for the lib/*.js files, would be great.

const path = require('path')
const { exist } = require('./util')

const rimraf = (p) => {
Copy link
Contributor

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": "",
Copy link
Contributor

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",
Copy link
Contributor

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"
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants