Add verbose option to search arguments as well as command names #14
Conversation
|
Excellent! |
|
Hi, Just wondering if you've had a chance to look at this yet? Daniel |
sindresorhus
changed the title
cli.js
Outdated
| @@ -14,6 +14,7 @@ const cli = meow(` | |||
|
|
|||
| Options | |||
| -f, --force Force kill | |||
| -v, --verbose Include arguments in process search | |||
There was a problem hiding this comment.
Should be two spaces between the flag and description, and you need to align the above flag.
There was a problem hiding this comment.
Better description:
Show process arguments
cli.js
Outdated
| inquirer.registerPrompt('autocomplete', require('inquirer-autocomplete-prompt')); | ||
| const verbose = flags.verbose || false; |
There was a problem hiding this comment.
You can set default values with minimist instead.
cli.js
Outdated
|
|
||
| return inquirer.prompt([{ | ||
| name: 'processes', | ||
| message: 'Running processes:', | ||
| type: 'autocomplete', | ||
| source: (answers, input) => Promise.resolve().then(() => filterProcesses(input, processes)) | ||
| source: (answers, input) => Promise.resolve().then(() => filterProcesses(input, processes, verbose)) |
There was a problem hiding this comment.
I would continue passing the flags object down here to make it easier to add more options in the future.
|
Generally looks good :) You should limit the length though. Chrome has some crazy long process arguments: |
|
I know it's verbose mode, but I think we should simplify the apps in this mode:
|
|
@sindresorhus this comment #13 (comment) regarding the chrome processes might be helpful. |
|
Have updated based on feedback; yet to sort out extracting the process name from the args and truncating the macOS .app insides. Will take a look at that later today hopefully. Updates address help output, truncating overly verbose procs to one line, passing flags throughout the app |
cli.js
Outdated
| const textLength = lineLength - 3; | ||
| const front = Math.ceil(textLength / 2); | ||
| const back = Math.floor(textLength / 2); | ||
| return `${text.substr(0, front)}...${text.substr(text.length - back)}`; |
There was a problem hiding this comment.
Would prefer to use a module for this, like: https://github.com/sindresorhus/cli-truncate
| @@ -13,7 +13,8 @@ const cli = meow(` | |||
| $ fkill [<pid|name> ...] | |||
|
|
|||
| Options | |||
| -f, --force Force kill | |||
| -f, --force Force kill | |||
| -v, --verbose Show process arguments | |||
There was a problem hiding this comment.
You also need to update the readme output.
|
Changes to ps-list to extract args from command will solve issues such as:
(I was thinking of removing all text after Excluding the integration with future |
cli.js
Outdated
| })); | ||
| .map(proc => { | ||
| const lineLength = process.stdout.columns || 80; | ||
| const margins = 4 + proc.pid.toString().length; |
There was a problem hiding this comment.
What is 4? Can you make it a constant with a descriptive name? It seem arbitrary without more context. Also note that cliTruncate accounts for the dots, so you don't have to do that.
cli.js
Outdated
| const margins = 4 + proc.pid.toString().length; | ||
| const length = lineLength - margins; | ||
| const name = cliTruncate(flags.verbose ? proc.cmd : proc.name, length, { position: 'middle' }); | ||
| return { name: `${name} ${chalk.dim(proc.pid)}`, pid: proc.pid }; |
There was a problem hiding this comment.
Put the properties on separate lines for this, for readability reasons
|
You need to resolve the merge conflict. |
|
Yay! Thanks for working on this @coffeedoughnuts. Good stuff.
|
|
@coffeedoughnuts Would you be interested in being a maintainer on this project? You did such a great job with this PR. No worries if not though :) |
|
@sindresorhus hey - i've not maintained an open source project before; what exactly would it involve? (Just responding to issues and approving PRs?) Would be interested though; mainly contributed to this one because i'm so sick of |
|
Being kind to people is always the first thing to do :). And then yes, helping to triage issues and review PRs. I always let @sindresorhus make the final decision though as it's his project so he's the one to decide on new features etc. Something I forgot to mention? |
|
well - happy to help in any way i can :) |
|
@coffeedoughnuts Yes, what Sam said. Just helping out in any way you feel like :) |
|
Hey guys, just testing this on my local machine and ran into some problems. It seems fkill is having trouble killing some processes when I search for them by argument (the procs are owned by me, and are killable with ➜ ps ax | grep -v grep | grep runserver
90766 s003 S+ 0:01.05 python ./manage.py runserver
90769 s003 S+ 11:45.73 /Users/squash/.virtualenvs/grater/bin/python ./manage.py runserver
➜ fkill -v
? Running processes: runserv
? Running processes: python ./manage.py runserver 90766
AggregateError:
Error: Killing process python ./manage.py runserver 90766 failed: No matching processes belonging to you were found
at Promise.all.then (/Users/squash/.config/yarn/global/node_modules/fkill/index.js:41:10)
at Promise.all.then (/Users/squash/.config/yarn/global/node_modules/fkill/index.js:41:10)
[1] |
This feature allows you to run
fkill -vand then when searching for processes to kill, it will include the full proc command (including arguments)This is useful when I have a daemonic process, like a dev web server, that I want to kill, but was launched with
python start_web_server.pyfor example.Fixes #13
Fixes #18