config.fatal now throws an exception #357
Conversation
|
@jrmclaurin did you make your README changes in the file that the README is generated from? I think it's shell.js. @ariporad what do you think of this? I don't like having 2 options for basically the same thing. Maybe throwing is better behavior? So we could change Update: I originally said config.error, when I really meant |
|
Hi @nfischer, I hadn't noticed that README was generated, so I updated it directly. Sorry about that. |
|
Could you move the changes to the file its generated from? Otherwise CI won't pass. The issue is that regenerating docs will just wipe out your changes, which isn't what we want |
|
I say we make config.fatal just do this. I can't see any reason why that |
|
@jrmclaurin would you be able to refactor config.fatal? |
|
@nfischer Sure thing, I'll do that refactor, and put the docs in the right place. Thanks!! |
|
Awesome! Thanks, @jrmclaurin! If you can, please squash your commits when finished (or we can do that for you, if you prefer). This should be a good addition |
|
OK @nfischer, I refactored to change the behavior of config.fatal, and update unit tests to match. I believe I squashed the commits correctly, but let me know if I did anything wrong. Thanks everyone for your open-mindedness about this issue! |
|
I forgot about @nfischer's suggestion to exit silently if both config.fatal and config.silent are set. This is a bit of conjecture, but if someone sets both config.silent and config.fatal, they probably want config.silent to eliminate console noise when everything's OK, but they'll expect config.fatal to still throw when an error happens. So should we leave it like it is (fatal always throws), and anyone who wants "silent fatal" behavior can wrap their code in try / catch and swallow errors? Making config.fatal stronger than config.silent seems like the safer choice. |
|
As it is, the only time commands output to the console is for errors (and for That's just a thought though, and I don't mind much one way or another, since I don't think backward compatibility in this sense is terribly important. The try-catch approach is also good for suppressing errors, and having one approach probably makes things a little cleaner. |
| assert.equal(result.stdout, ''); | ||
| assert.equal(result.stderr, 'ls: no such file or directory: file_doesnt_exist\n'); | ||
| assert(result.stderr.indexOf('Error: ls: no such file or directory: file_doesnt_exist') >= 0); |
There was a problem hiding this comment.
Why does this have "Error: " prepended to the front?
There was a problem hiding this comment.
Instead of directly printing the message to stderr, we're now throwing an Error, so we end up at node's uncaught error handling. That's why you end up with "Error:" in front of the message.
|
OK, I made that node.js version check in test/set.js actually work right :-) |
| assert.throws(function() { | ||
| shell.exec('asdfasdf'); // could not find command | ||
| }, function(err) { | ||
| return err.message === 'exec: internal error'; |
There was a problem hiding this comment.
Two things:
- I'm pretty sure you can just pass in the expected error message here, and it will compare.
- Is there any way to make the error message a little more descriptive?
|
Looks good,but I have one small nitpick. |
|
@ariporad You were right that assert.throws can just look at the message, so I made that change. Do you want a more detailed message when I'm creating the Error? I can't think of anything else I could put in there that would be helpful. |
|
@jrmclaurin: I was thinking something like "Command not found" |
|
@ariporad Good point--I hadn't realized that the error message from the command isn't making it into the thrown error. To fix that problem, it looks like src/exec.js would need to scrape some stderr and / or stdout, and stuff it in the message that's being sent to common.error. Here's the scene of the crime, src/exec.js line 151: if (code !== 0) { |
|
@jrmclaurin: Ok. I'm working on re-doing exec, so don't worry. LGTM! |
|
@nfischer: feel free to merge. |
|
I'll test this out today and merge if I have no issues |
nfischer
changed the title
|
LGTM! I renamed the PR to better describe the change that's being merged, since it's deviated a bit from the original intent. Thanks, @jrmclaurin! |
config.fatal now throws an exception
Hi everyone!
This PR adds a 'config.throw' option to throw errors from shell commands. Config.fatal is useful, but it doesn't log stack traces, which makes deep errors hard to find. This 'throw' option is useful in synchronous scripts where you have try / catch handling, or you want the default behavior of printing a full stack trace and exiting.
Thanks!!