Throw error in sync method - fixes #66 #67

SamVerschueren · 2016-12-20T14:15:24Z

This should fix #66 by throwing an Error object if something is written to stderr. Also added a test for this.

SamVerschueren · 2016-12-20T14:19:33Z

Oh, I totally missed test.js in the root. Let me move the test I wrote to that file.

SamVerschueren · 2016-12-20T14:40:18Z

Odd, this works on my Mac and on Windows but apparently not on Linux...

SamVerschueren · 2016-12-21T12:41:54Z

The test doesn't seem to be correct. Is there any other way I can make sure something is printed to stderr instead?

sindresorhus · 2016-12-21T16:35:06Z

I think you misunderstood. It shouldn't throw when something is outputted on stderr. stderr is not just for errors, it's an alternative stream. It should throw when there's an error (.error has something).

SamVerschueren · 2016-12-22T07:41:46Z

Oh yes, you're totally right! My bad.

I fixed the implementation but couldn't come up with a command that had an error object on Windows. I'm really not familiar with that system. Everything I tried just exited with status 1 and data in the stderr. So if you have any better ideas on which command we could use that fails on Windows, let me know and I'll fix the tests.

sindresorhus · 2016-12-22T08:43:42Z

Try spawning something that doesn't exist.

SamVerschueren · 2016-12-22T09:32:28Z

Unfortunately that doesn't give an error...

$ cd foo

{ status: 1,
  signal: null,
  output: [ null, '', 'The system cannot find the path specified.\r\n' ],
  pid: 684,
  stdout: '',
  stderr: 'The system cannot find the path specified.'
}

$ foo

{ status: 1,
  signal: null,
  output:
   [ null,
     '',
     '\'foo\' is not recognized as an internal or external command,\r\noperable program or batch file.\r\n' ],
  pid: 1760,
  stdout: '',
  stderr: '\'foo\' is not recognized as an internal or external command,\r\noperable program or batch file.'
}

sindresorhus · 2016-12-25T18:47:10Z

That sounds like a Node.js bug. The status (exit code) is not 0, so it should be an error. You could check the status though.

SamVerschueren · 2016-12-25T18:52:51Z

Do you mean in the code? Something like this:

if (result.error || result.status !== 0) {
    throw result.error || new Error(result.stderr);
}

sindresorhus · 2016-12-25T19:02:28Z

👍

SamVerschueren · 2016-12-26T21:15:01Z

Finally got it working on both Travis and AppVeyor :p.

sindresorhus · 2016-12-26T23:45:23Z

index.js

@@ -231,6 +231,10 @@ module.exports.sync = (cmd, args, opts) => {

 	const result = childProcess.spawnSync(parsed.cmd, parsed.args, parsed.opts);

+	if (result.error || result.status !== 0) {
+		throw result.error || new Error(result.stdout === '' ? result.stderr : result.stdout);


Wrap it in parens for clarity:

throw (result.error || new Error(result.stdout === '' ? result.stderr : result.stdout));

Would it make sense to switch the stdout/stderr check to something like this

new Error(result.stderr === '' ? result.stdout : result.stderr);

Yes, actually, that would make sense.

sindresorhus · 2016-12-26T23:45:32Z

readme.md

@@ -95,6 +95,8 @@ Same options as [`child_process.execFileSync`](https://nodejs.org/api/child_proc

 Returns the same result object as [`child_process.spawnSync`](https://nodejs.org/api/child_process.html#child_process_child_process_spawnsync_command_args_options).

+This method throws an `Error` if something is written to `stderr`.


This needs to reflect the actual code.

LinusU · 2017-01-08T17:46:03Z

Can this be merged now? I would love to have it in ❤️

sindresorhus · 2017-01-08T18:24:10Z

It's waiting on #67 (comment)

SamVerschueren · 2017-01-08T18:42:30Z

@sindresorhus I fixed that. Forgot to notify you.

sindresorhus · 2017-01-09T09:34:57Z

Cool. Thanks Sam :)

SamVerschueren · 2017-01-09T10:03:00Z

You're welcome! And thank you for merging 🎉

SamVerschueren force-pushed the iss66 branch 2 times, most recently from a8a5684 to b4031c4 Compare December 20, 2016 14:21

SamVerschueren force-pushed the iss66 branch from b4031c4 to 70ed8e7 Compare December 22, 2016 07:39

SamVerschueren force-pushed the iss66 branch from 1c790d3 to 8bc78da Compare December 26, 2016 21:12

sindresorhus requested changes Dec 26, 2016

View reviewed changes

Throw error in sync method - fixes #66

c45c190

SamVerschueren force-pushed the iss66 branch from 9c27906 to c45c190 Compare January 6, 2017 07:27

sindresorhus approved these changes Jan 9, 2017

View reviewed changes

sindresorhus merged commit c8c9c7b into master Jan 9, 2017

sindresorhus deleted the iss66 branch January 9, 2017 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw error in sync method - fixes #66 #67

Throw error in sync method - fixes #66 #67

SamVerschueren commented Dec 20, 2016

SamVerschueren commented Dec 20, 2016

SamVerschueren commented Dec 20, 2016

SamVerschueren commented Dec 21, 2016 •

edited

Loading

sindresorhus commented Dec 21, 2016

SamVerschueren commented Dec 22, 2016

sindresorhus commented Dec 22, 2016

SamVerschueren commented Dec 22, 2016

sindresorhus commented Dec 25, 2016

SamVerschueren commented Dec 25, 2016

sindresorhus commented Dec 25, 2016

SamVerschueren commented Dec 26, 2016

sindresorhus Dec 26, 2016

SamVerschueren Dec 28, 2016

sindresorhus Jan 5, 2017

sindresorhus Dec 26, 2016

LinusU commented Jan 8, 2017

sindresorhus commented Jan 8, 2017

SamVerschueren commented Jan 8, 2017

sindresorhus commented Jan 9, 2017

SamVerschueren commented Jan 9, 2017 •

edited

Loading

		@@ -95,6 +95,8 @@ Same options as [`child_process.execFileSync`](https://nodejs.org/api/child_proc

		Returns the same result object as [`child_process.spawnSync`](https://nodejs.org/api/child_process.html#child_process_child_process_spawnsync_command_args_options).

		This method throws an `Error` if something is written to `stderr`.

Throw error in sync method - fixes #66 #67

Throw error in sync method - fixes #66 #67

Conversation

SamVerschueren commented Dec 20, 2016

SamVerschueren commented Dec 20, 2016

SamVerschueren commented Dec 20, 2016

SamVerschueren commented Dec 21, 2016 • edited Loading

sindresorhus commented Dec 21, 2016

SamVerschueren commented Dec 22, 2016

sindresorhus commented Dec 22, 2016

SamVerschueren commented Dec 22, 2016

sindresorhus commented Dec 25, 2016

SamVerschueren commented Dec 25, 2016

sindresorhus commented Dec 25, 2016

SamVerschueren commented Dec 26, 2016

sindresorhus Dec 26, 2016

Choose a reason for hiding this comment

SamVerschueren Dec 28, 2016

Choose a reason for hiding this comment

sindresorhus Jan 5, 2017

Choose a reason for hiding this comment

sindresorhus Dec 26, 2016

Choose a reason for hiding this comment

LinusU commented Jan 8, 2017

sindresorhus commented Jan 8, 2017

SamVerschueren commented Jan 8, 2017

sindresorhus commented Jan 9, 2017

SamVerschueren commented Jan 9, 2017 • edited Loading

SamVerschueren commented Dec 21, 2016 •

edited

Loading

SamVerschueren commented Jan 9, 2017 •

edited

Loading