Added verbosity flag (-v) for cp, mv and rm commands by nicky1038 · Pull Request #1129 · shelljs/shelljs · GitHub
Skip to content

Added verbosity flag (-v) for cp, mv and rm commands#1129

Open
nicky1038 wants to merge 1 commit into
shelljs:mainfrom
nicky1038:cp-mv-rm-verbose-output
Open

Added verbosity flag (-v) for cp, mv and rm commands#1129
nicky1038 wants to merge 1 commit into
shelljs:mainfrom
nicky1038:cp-mv-rm-verbose-output

Conversation

@nicky1038

@nicky1038 nicky1038 commented Aug 23, 2023

Copy link
Copy Markdown

Closes #1127

The suggested verbose output is not claimed to be fully identical to the corresponding output in the original commands from coreutils. Though it is very similar, I also tried to make as few edits as possible.

@nicky1038 nicky1038 force-pushed the cp-mv-rm-verbose-output branch 2 times, most recently from 5c298ee to dd92b29 Compare August 23, 2023 21:14
@nicky1038 nicky1038 force-pushed the cp-mv-rm-verbose-output branch from dd92b29 to 1e3df2a Compare August 23, 2023 21:35
@nicky1038

nicky1038 commented Aug 23, 2023

Copy link
Copy Markdown
Author

Comment thread src/rm.js
common.unlinkSync(file);
if (options.verbose) {
console.log("removed '" + file + "'");
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this so that file is a method parameter? I'd also suggest passing options as a second parameter and then moving this function definition to the global scope, similar to what you've done with handleFIFO().

Comment thread src/mv.js
cp({ recursive: true }, src, thisDest);
rm({ recursive: true, force: true }, src);
cp({ recursive: true, verbose: options.verbose }, src, thisDest);
rm({ recursive: true, force: true, verbose: options.verbose }, src);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 😄

Comment thread src/cp.js
}

if (options.verbose) {
console.log("copied '" + srcFile + "' -> '" + destFile + "'");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should drop the "copied" prefix. When I tested this on my linux machine, I don't see this in the output:

$ cp -vr src/ dest/
'src/' -> 'dest/'
'src/file.txt' -> 'dest/file.txt'
'src/sub1' -> 'dest/sub1'
'src/sub1/sub2' -> 'dest/sub1/sub2'
'src/sub1/sub2/file2.txt' -> 'dest/sub1/sub2/file2.txt'

On the other hand, keeping "copied" would be more consistent with the other commands, since those do say "removed" or "renamed" before the file name.

What do you think?

Comment thread src/rm.js
result = fs.rmdirSync(dir);
if (fs.existsSync(dir)) throw { code: 'EAGAIN' };
if (verbose) {
console.log("removed directory'" + dir + "'");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space between directory and '

@codecov

codecov Bot commented Aug 28, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@f7a7c75). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/rm.js 66.66% 6 Missing ⚠️
src/cp.js 50.00% 2 Missing ⚠️
src/mv.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1129   +/-   ##
=======================================
  Coverage        ?   96.36%           
=======================================
  Files           ?       36           
  Lines           ?     1375           
  Branches        ?        0           
=======================================
  Hits            ?     1325           
  Misses          ?       50           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nfischer

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request (cp): -v flag for verbose output

2 participants