Add `verbose` option #7

MarkTiedemann · 2017-03-03T17:10:26Z

My first pull request. :)

Adds a verbose option to improve performance (see: https://github.com/sindresorhus/tasklist/issues/6
Adds an apps option (to return only Windows Store Apps)
Makes the filter option accept not only a filter array, but also a single filter string

MarkTiedemann · 2017-03-03T17:18:45Z

CI failed for nodejs_version=0.12 with the following output:

const debug = require('debug')('xo');
^^^^^
SyntaxError: Use of const in strict mode.

Should support for Node v0.12 get dropped or should the xo devDependency be downgraded?

sindresorhus · 2017-03-06T19:55:55Z

Should support for Node v0.12 get dropped or should the xo devDependency be downgraded?

I will do it after this PR. Just ignore Node.js 0.12 failures for now.

sindresorhus · 2017-03-06T19:52:04Z

index.js

+		args.push('/apps');
+	}
+
+	if (Array.isArray(opts.filter)) {


I prefer to keep it Array only. I don't like mixing types with little benefit.

sindresorhus · 2017-03-06T19:53:28Z

readme.md

+
+Type: `boolean`
+
+Return verbose results (default: `false`).


Write the default value like done here: https://github.com/sindresorhus/boxen#borderstyle

sindresorhus · 2017-03-06T19:55:12Z

readme.md

-		status: 'Running',
-		username: 'SINDRESORHU3930\\sindre'
-		cpuTime: 0,  // seconds
-		windowTitle: 'Task Host Window'


I think it's useful for people to see the properties like this with values, so maybe add another example in the verbose option section and show these.

- Make test cases check all properties - Revert making `memUsage` a `verbose` property (which it is not) - Revert making `filter` option accept strings - Add example to `verbose` option documentation - Change formatting of default values in documentation

MarkTiedemann · 2017-03-06T23:19:04Z

I have made the changes you requested and even fixed a bug in my code. :)

Make test cases check all properties
Revert making memUsage a verbose property (which it is not)
Revert making filter option accept strings
Add example to verbose option documentation
Change formatting of default values in documentation

MarkTiedemann · 2017-03-07T09:57:15Z

Just noticed CI failed, and for good reason:

It seems like windowTitle won't be set in the CI env
The apps version returns different rows:
- Specifically, it does not have sessionName and sessionNumber, but instead it has packageName
- In verbose mode, it has both sessionName and sessionNumber as well as packageName

Gonna update the PR as soon as possible. :)

MarkTiedemann · 2017-03-29T11:51:29Z

Earlier CI failed on Node 0.12 with linting errors, now it failed on Node 4, too.

sindresorhus · 2017-04-23T16:50:06Z

@MarkTiedemann What's the status of this PR? :)

MarkTiedemann · 2017-04-24T10:07:30Z

Well, I (almost) forgot about the PR. ;)

But thanks to your question, last night I spent 2-3 hours revisiting, upgrading, re-testing, fixing, and completely refactoring the old code. :)

Not 100% happy with the results yet, and some documentation is still missing, but I'm probably gonna push the results in the next few days.

PS: Last time I worked on this PR a couple of weeks ago, I created a streaming version of this module: tasklist-stream. I think once this PR is done, we could perhaps merge both in some way (they do "share" a lot of code already ...).

This commit includes the following changes: - Update Node versions in appveyor.yml - Update dependencies - Use const instead of var - Use () => {} instead of function() {} - Remove Promise ponyfill - Remove 'N/A' check due to failure on non-English systems - Make the tests cases cover all returned tasks

MarkTiedemann · 2017-04-24T23:36:35Z

@sindresorhus Here's my refactoring:

Update Node versions in appveyor.yml
Update dependencies
Use const instead of var
Use () => {} instead of function() {}
Remove Promise ponyfill
Remove 'N/A' check due to failure on non-English systems
Make the tests cases cover all returned tasks

Hope you like it. :)

sindresorhus

Looks really good. Just some minor nitpick.

sindresorhus · 2017-04-30T11:24:16Z

test.js

-	t.is(typeof d.pid, 'number');
-	t.is(typeof d.memUsage, 'number');
-});
+const makeTest = (options, t) => {


Can you use a test macro instead?

Been using ava since .9, I totally missed that you support test macros now. Thanks for the advice!

sindresorhus · 2017-04-30T11:24:38Z

test.js

+const makeTest = (options, t) => {
+	return tasklist(options).then(tasks => {
+		t.true(tasks.length > 0);
+		tasks.forEach(task => {


Use a for-of loop.

sindresorhus · 2017-04-30T11:24:48Z

test.js

-import test from 'ava';
-import fn from './';
+const test = require('ava');
+const tasklist = require('./');


Don't change this.

I didn't like the inconsistency of having require statements in the index file and import statements in the test file. But I guess your plan is to change to import statements everywhere in the long run, so yeah, gladly changing it back. :)

sindresorhus · 2017-04-30T11:24:58Z

package.json

-    "ava": "*",
-    "xo": "*"
+    "ava": "^0.19.1",
+    "xo": "^0.18.1"


Don't change this.

sindresorhus · 2017-04-30T11:25:09Z

package.json

  },
  "scripts": {
-    "test": "xo && ava"
+    "test": "xo --fix && ava --verbose"


Don't change this.

sindresorhus · 2017-04-30T11:25:19Z

package.json

@@ -10,10 +10,10 @@
    "url": "sindresorhus.com"
  },
  "engines": {
-    "node": ">=0.10.0"
+    "node": ">=4.0.0"


"node": ">=4"

sindresorhus · 2017-04-30T11:25:52Z

test.js

-	t.is(typeof d.memUsage, 'number');
-});
+const makeTest = (options, t) => {
+	return tasklist(options).then(tasks => {


Use async/await

sindresorhus · 2017-04-30T11:26:24Z

index.js

-			args.push('/fi', el);
-		});
+	if (Array.isArray(options.filter)) {
+		options.filter.forEach(fi => args.push('/fi', fi));


Use for-of loop.

There was no for-loop before. I'm gladly changing it, but may I ask why?

It's faster. I'm trying to use it over forEach whenever possible. So for consistency too. And I don't like inline arrow functions for things that doesn't return anything. Makes the code harder to read.

Okay, I understand that. Thanks for explaining!

PS: I have fixed the requested changes. :)

sindresorhus · 2017-04-30T11:27:18Z

index.js

 	if (process.platform !== 'win32') {
 		return Promise.reject(new Error('Windows only'));
 	}

-	opts = opts || {};
+	const options = opts || {};


Don't change this. I have it like this so it will be super easy to change to default parameters when I can target Node.js 6.

I didn't think about that. Thanks for explaining. :)

sindresorhus · 2017-05-01T09:00:55Z

Looks great. Thank you @MarkTiedemann :)

sindresorhus · 2017-05-01T09:02:04Z

Would you be interested in being a maintainer here? I don't think this module requires much more work, so it's mostly just to highlight your contribution here in the readme.

MarkTiedemann · 2017-05-01T10:21:33Z

Yeah, sure. :) Thanks a lot!

Well, there's some work I'd like to do here in the time to come:

Implement the /apps flag
Implement the /m module flag
Implement the /svc service flag
Make a streaming interface available (I already created a prototype at tasklist-stream, but there's more work to be done there, too. And I'm not sure how to "merge" both modules - if they should be merged at all.
As per tasklist /?, windowTitle and status filters are not supported when querying a remote machine. I'd like to implement a check for that, and also test whether this case breaks the code of this module in any way.

sindresorhus · 2017-05-01T10:46:39Z

Awesome! Could you put the above into an issue with checklists?

MarkTiedemann · 2017-05-01T13:53:18Z

Yeah, sure. See: #8.

Add verbose option, add apps option, and update filter option

65ff7b8

sindresorhus requested changes Mar 6, 2017

View reviewed changes

Improve pull request

ae80ff2

- Make test cases check all properties - Revert making `memUsage` a `verbose` property (which it is not) - Revert making `filter` option accept strings - Add example to `verbose` option documentation - Change formatting of default values in documentation

MarkTiedemann added 2 commits March 29, 2017 13:40

Remove apps option

439254d

Don't test for windowTitle in CI env

1e8aacd

sindresorhus requested changes Apr 30, 2017

View reviewed changes

Fix requested changes

c1f4229

sindresorhus approved these changes May 1, 2017

View reviewed changes

sindresorhus changed the title ~~Add verbose option, add apps option, and update filter option~~ Add verbose option May 1, 2017

sindresorhus merged commit 38011ad into sindresorhus:master May 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `verbose` option #7

Add `verbose` option #7

MarkTiedemann commented Mar 3, 2017

MarkTiedemann commented Mar 3, 2017

sindresorhus commented Mar 6, 2017

sindresorhus Mar 6, 2017

sindresorhus Mar 6, 2017

sindresorhus Mar 6, 2017

MarkTiedemann commented Mar 6, 2017

MarkTiedemann commented Mar 7, 2017

MarkTiedemann commented Mar 29, 2017

sindresorhus commented Apr 23, 2017

MarkTiedemann commented Apr 24, 2017

MarkTiedemann commented Apr 24, 2017

sindresorhus left a comment

sindresorhus Apr 30, 2017

MarkTiedemann Apr 30, 2017

sindresorhus Apr 30, 2017

sindresorhus Apr 30, 2017

MarkTiedemann Apr 30, 2017

sindresorhus Apr 30, 2017

sindresorhus Apr 30, 2017

sindresorhus Apr 30, 2017

sindresorhus Apr 30, 2017

sindresorhus Apr 30, 2017

MarkTiedemann Apr 30, 2017

sindresorhus Apr 30, 2017

MarkTiedemann Apr 30, 2017

sindresorhus Apr 30, 2017

MarkTiedemann Apr 30, 2017

sindresorhus commented May 1, 2017

sindresorhus commented May 1, 2017

MarkTiedemann commented May 1, 2017

sindresorhus commented May 1, 2017

MarkTiedemann commented May 1, 2017

Add verbose option #7

Add verbose option #7

Conversation

MarkTiedemann commented Mar 3, 2017

MarkTiedemann commented Mar 3, 2017

sindresorhus commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkTiedemann commented Mar 6, 2017

MarkTiedemann commented Mar 7, 2017

MarkTiedemann commented Mar 29, 2017

sindresorhus commented Apr 23, 2017

MarkTiedemann commented Apr 24, 2017

MarkTiedemann commented Apr 24, 2017

sindresorhus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 1, 2017

sindresorhus commented May 1, 2017

MarkTiedemann commented May 1, 2017

sindresorhus commented May 1, 2017

MarkTiedemann commented May 1, 2017

Add `verbose` option #7

Add `verbose` option #7