Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add concurrency option #69

Merged
merged 6 commits into from Aug 3, 2019
Merged

Add concurrency option #69

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7ac71f6
added support for p-all, properly receives and uses concurrency optio…
threequartersjohn Jul 31, 2019
2f55ac3
updated declarations, cleaner code, improved readme
threequartersjohn Jul 31, 2019
1c884c9
fixed issue where destination path wasn't being returned
threequartersjohn Jul 31, 2019
4f4b875
fixed readme
threequartersjohn Jul 31, 2019
7a95e94
simplified setting concurrency option default value
threequartersjohn Aug 1, 2019
f861103
fixed declaration, fixed for passthrough to p-all validation, added t…
threequartersjohn Aug 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions index.d.ts
View file
Open in desktop
Expand Up @@ -32,6 +32,13 @@ declare namespace cpy {
```
*/
readonly rename?: string | ((basename: string) => string);

/**
Number of files being copied concurrently.

@default "(os.cpus().length || 1) * 2"
threequartersjohn marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly concurrency?: number;
}

interface ProgressData {
Expand Down
31 changes: 21 additions & 10 deletions index.js
View file
Open in desktop
@@ -1,6 +1,8 @@
'use strict';
const EventEmitter = require('events');
const path = require('path');
const os = require('os');
const pAll = require('p-all');
const arrify = require('arrify');
const globby = require('globby');
const cpFile = require('cp-file');
Expand Down Expand Up @@ -31,6 +33,13 @@ const preprocessDestinationPath = (source, destination, options) => {

module.exports = (source, destination, options = {}) => {
sindresorhus marked this conversation as resolved.
Show resolved Hide resolved
const progressEmitter = new EventEmitter();
let concurrencyDegree;
Copy link

Choose a reason for hiding this comment

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

Why did you create a new var to make options.concurrency default to (os.cpus().length || 1) * 2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the new variable to make it explicit in reading what the value meant, do you think it's unnecessary? I think having it like this also makes it easier to find the default value. I could make it so no variable is added, or changing it such that the default value is abstracted to a variable and pass it with {concurrency: options.concurrency || defaultConcurrency} instead, my only concern was the readability of the code.

Copy link

Choose a reason for hiding this comment

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

I think what @hugomrdias is saying (and giving you a hint) is that you can simply the code. For example, you may leverage object spread to have default options being mixed in with the ones the user supplies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, not sure what you mean with object spread, since this code change operates with only one of the options. The other options are handled in other functions.

I could default it like:
module.exports = (source, destination, options = {concurrency: (os.cpus().length || 1 * 2}) => [...]
...which also makes the default value easy to find. I'm not sure if this would work. Do you think that would be better?

Copy link

@satazor satazor Aug 1, 2019 •
edited

Choose a reason for hiding this comment

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

You are right in the sense that each option is currently being handled in each function, so perhaps lets keep the consistency.

Still, we may simplify with:

    const concurrency = options.concurrency || (os.cpus().length || 1) * 2;

Also, by renaming the variable to concurrency from concurrencyDegree, you may use it directly when passing it to pAll.

Other than that, the PR looks good. However, I suggest adding a simple test (smoke test) to see if the concurrency is being respected.


if (options.concurrency) {
concurrencyDegree = options.concurrency;
} else {
concurrencyDegree = (os.cpus().length || 1) * 2;
}

const promise = (async () => {
source = arrify(source);
Expand Down Expand Up @@ -84,18 +93,20 @@ module.exports = (source, destination, options = {}) => {
}
};

return Promise.all(files.map(async sourcePath => {
const from = preprocessSourcePath(sourcePath, options);
const to = preprocessDestinationPath(sourcePath, destination, options);
return pAll(files.map(sourcePath => {
return async () => {
const from = preprocessSourcePath(sourcePath, options);
const to = preprocessDestinationPath(sourcePath, destination, options);

try {
await cpFile(from, to, options).on('progress', fileProgressHandler);
} catch (error) {
throw new CpyError(`Cannot copy from \`${from}\` to \`${to}\`: ${error.message}`, error);
}
try {
await cpFile(from, to, options).on('progress', fileProgressHandler);
} catch (error) {
throw new CpyError(`Cannot copy from \`${from}\` to \`${to}\`: ${error.message}`, error);
}

return to;
}));
return to;
};
}), {concurrency: concurrencyDegree});
})();

promise.on = (...arguments_) => {
Expand Down
4 changes: 4 additions & 0 deletions index.test-d.ts
View file
Open in desktop
Expand Up @@ -23,6 +23,10 @@ expectType<Promise<string[]> & ProgressEmitter>(
expectType<Promise<string[]> & ProgressEmitter>(
cpy('foo.js', 'destination', {overwrite: false})
);
expectType<Promise<string[]> & ProgressEmitter>(
cpy('foo.js', 'destination', {concurrency: 2})
);


expectType<Promise<string[]>>(
cpy('foo.js', 'destination').on('progress', progress => {
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -45,7 +45,8 @@
"arrify": "^2.0.1",
"cp-file": "^7.0.0",
"globby": "^9.2.0",
"nested-error-stacks": "^2.1.0"
"nested-error-stacks": "^2.1.0",
"p-all": "^2.1.0"
},
"devDependencies": {
"ava": "^2.1.0",
Expand Down
7 changes: 7 additions & 0 deletions readme.md
View file
Open in desktop
Expand Up @@ -94,6 +94,13 @@ const cpy = require('cpy');
})();
```

##### concurrency

Type: `number`<br>
Default: `(os.cpus().length || 1) * 2`

Number of files being copied concurrently.


## Progress reporting

Expand Down