-
-
Notifications
You must be signed in to change notification settings - Fork 161
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.
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
add support for killing by port (using a colon to indicate port usage) #27
Changes from 3 commits
242e266
480c0a6
5e2cd95
1b2bc93
2b9b099
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ const psList = require('ps-list'); | |
const numSort = require('num-sort'); | ||
const escExit = require('esc-exit'); | ||
const cliTruncate = require('cli-truncate'); | ||
const portToProc = require('portproc').portToProc; | ||
|
||
const cli = meow(` | ||
Usage | ||
|
@@ -96,5 +97,18 @@ function handleFkillError(processes) { | |
if (cli.input.length === 0) { | ||
init(cli.flags); | ||
} else { | ||
fkill(cli.input, cli.flags).catch(() => handleFkillError(cli.input)); | ||
let pids = []; | ||
const promises = []; | ||
for (let i = 0; i < cli.input.length; i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just do |
||
if (cli.input[i][0] === ':') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made a little regex for this https://github.com/kevva/port-regex :). |
||
promises.push(portToProc(parseInt(cli.input[i].slice(1), 0))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} else { | ||
pids.push(cli.input[i]); | ||
} | ||
} | ||
Promise.all(promises).then(promisedPids => { | ||
promisedPids = promisedPids.filter(pid => pid); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are you filtering here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure; I think if you ask to kill a valid target and an invalid target then the operation shouldn't fail; the valid target should be killed. But silently failing doesn't feel great, I agree. As a command line tool, rather than a library, perhaps it should be a little more permissive to user input? What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, your module should reject and we should handle it in here (i.e not failing), imo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't rejecting a Promise like throwing an error? I'm not sure it's an error if you ask for the process using port X and there is no process using that port. That feels to me like it should return undefined. If we want to treat undefined in this use case as an error, we can do - though I think i'd be more in favour or silently ignoring the command. At least if there's positive hits in the request too? Am I making sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, maybe. I usually throw but I see where you coming from. Maybe @sindresorhus @SamVerschueren has any thoughts? |
||
pids = pids.concat(promisedPids); | ||
fkill(pids, cli.flags).catch(() => handleFkillError(cli.input)); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would write it as: if (cli.input.length === 0) {
init(cli.flags);
} else {
Promise.all(cli.input.map(x => x[0] === ':' ? portToProc(parseInt(x.slice(1), 10)) : x))
.then(pids => fkill(pids, cli.flags).catch(() => handleFkillError(cli.input)));
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall I prefer this approach; I'll update the branch |
||
} |
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.
Would have been more readable as:
Not really worth saving a few characters for limited gain.
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.
Can you extract the CLI in
portproc
into a separate package? I don't want to needlessly havecommander
in the dependency tree. See: sindresorhus/ama#17