Implementing error codes and adding the errorCode() function #269

nfischer · 2015-12-30T13:35:48Z

This implements error codes for the builtin commands. Each command will now have an associated error code (common.status.code). I changed many of the commands to return the code that the corresponding Linux command would return (ex. ls('asdfasdf') returns 2).

This fixes a bug with exec() as well. Previously, you would see the following results:

$ cat test.js
require('shelljs/global');
exec('exit 5'); // or any code besides 1, 2, or 126+
if (error())
 echo('caught the error');
else
 echo('this error slips by');

$ shjs test.js
this error slips by

Any non-zero exit code means that error() will return a truthy value.

If config.fatal is set, then the process will exit with the last return code that was set, emulating Bash's behavior.
This adds the errorCode() function to access the last error code, which is somewhat of a substitute for Bash's $? variable. This is good if a command's output may vary, but its return codes can be relied upon to stay consistent (or if output is inconvenient to type). It can be used like so:
```
require('shelljs/global');
var assert = require('assert');
ls('missing_file1');
var str1 = error(),
   code1 = errorCode();
ls('missing_file2');
var str2 = error(),
   code2 = errorCode();
assert.ok(str1 !== str2);
assert.ok(code1 === code2);
```

nfischer · 2016-01-11T09:28:26Z

I've modified some of the calls to error() to explicitly use code 1 where appropriate (that's the default code). This makes for a messier diff, since it results in many files being modified.

Because the default error code is 1, I can actually go back and revert these lines, and they should have the same behavior. This cleans up the diff, but might make things more confusing (since it relies on that being the default). Let me know if this is preferred (this is actually the way I'm leaning).

ariporad · 2016-01-11T14:33:38Z

src/common.js

This should be _code != 0

I wrote it like this for two reasons:

This prefers non-default return codes. If a command has two errors (and fatal === false, so it doesn't exit yet), and one is a non-default return code, then it prefers that value overall (which probably tells the user more useful information).

This gives the user a way to send warning messages (output like an error, but don't change the status code) by providing _code === 0

I thought about changing it to if (state.errorCode === 0 || state.errorCode === DEFAULT_RETURN_CODE) instead, but that would allow error('...', 0) to wipe out a previous non-zero code, which it probably shouldn't do.

What do you think?

ariporad · 2016-01-11T14:42:43Z

Looks Great, Just a few minor things I pointed out, and two bigger ones too:

Why not just have error() return the code, and get rid of errorCode? Any non-zero value is truthy, so it would still work, and that would make it equivalent to bash's $?.
Can you split this into two commits? One for the implimentation, and one for switching over all the functions.

Otherwise, looks great!

nfischer · 2016-01-11T18:18:47Z

Thanks for the feedback! I responded inline to the minor comments. In response to the major comments:

I thought about that with error(). I agree that the conditionals if (error()) and if (errorCode()) will behave the same. My concern is that error() already exists and provides a useful functionality in returning the error string. I'm not sure if anyone relies on it for its string value. One option is to rename error() to errorMsg() and errorCode() will become error(), but that may still break some scripts.
I'll split this up and rebase in a moment.

Also, what do you think about camel case for errorCode()? I was also thinking of naming it errorcode(). This would be the only function in the API that is camel case.

We might also consider adding a warn() function that outputs to stderr without changing the error code. This would be very simple to implement with things the way they are written.

nfischer · 2016-01-11T19:35:02Z

@ariporad I updated this and rebased off master. I reverted most of the calls to error(...1, true), but left my changes for the ones that change the status code to a non-default value. I can go back if and put those in a separate commit if you still prefer that (although this is enough to clean up the diff considerably).

If it looks good, feel free to merge. If not, leave some comments and I'll address them.

nfischer · 2016-01-15T05:44:34Z

@ariporad I think this is ready for review now

ariporad · 2016-01-17T18:09:21Z

@nfisher: Sorry for the delay.

I still think that we should just make error() return the error code. exec().stderr is there if you need the error string. This is what breaking changes are for.

And we should defiantly use cammelCase. What do we use now?

nfischer · 2016-01-17T19:20:30Z

Right now everything is lower case. "tempdir" is an example.

I agree that works well for exec (and is more intuitive), but I think someone may still want this feature for a non-exec function. Maybe we name this to error() and rename that to errorMsg()?

Or we refactor things heavily so that every command returns an object that has a stderr field. I'm working right now to have each command return objects instead of simple strings (which should allow for pipes), and could add the stderr field. This isn't my preferred method though, since I think relying on error messages is fairly fragile (especially since we just fixed bugs related to error messages)

ariporad · 2016-01-17T23:32:33Z

@nfischer: I think that we should wait till me and you and @arturadib (If he wants) to get together in gitter. It kind of revolves around how much we want to mess with shellljs. (I personally want to do a large refactor, and to standardize everything. Then we can just to a breaking change).

nfischer · 2016-01-18T01:23:05Z

👍

nfischer · 2016-01-24T23:32:48Z

Rebased off master

Implement error codes internally. The script will exit with the status of the last command. The default code for errors is 1.

ariporad · 2016-01-28T03:31:57Z

@nfischer: Since we're not meeting with Artur, I think we should just refactor everything to return an object with .stderr/.stdout, and have error() return the exit code.

nfischer · 2016-01-28T03:33:49Z

My only concern is that it would break a lot of backwards compatibility. I agree it's a cleaner API, but do you think enough people are relying on the old behavior?

I can do that refactor either way, though 👍

ariporad · 2016-01-31T17:05:52Z

@nfischer: Hmm... My comment didn't go through.

I meant to say (like 4 days ago) that I think breaking changes were invented for a reason.

nfischer · 2016-01-31T19:24:15Z

Hmm ok. It'll take me a couple days to sort out the bugs I ran into from the ShellString refactor. I'll try to prioritize that and make a separate PR for that, and then we can add error codes on top of that.

ariporad · 2016-02-14T19:46:54Z

@nfischer: What's the status on this? Also, could you rebase off master?

nfischer · 2016-02-15T03:18:51Z

@ariporad still in progress. I can rebase off master, but I think you'd prefer a different interface to expose return codes, so I'm waiting until I make some other changes. I'd at least like a stderr field being returned before we get rid of the current functionality of error()

ariporad · 2016-02-19T22:20:30Z

@nfischer: bump.

nfischer · 2016-02-19T22:30:43Z

@ariporad I think we should wait until #360 is merged, and then revisit alternate implementations. The way I see it it is that we can either do:

each ShellString has a code attribute (this wouldn't work well for commands like cp() that don't return anything)
error() returns the code, and we rely on the stderr attribute to get the error message (this also doesn't work well for commands like cp())

ariporad · 2016-02-24T19:28:22Z

@nfischer: Ok, since #360 is merged, can we move forward with having error return the error code?

nfischer · 2016-02-24T20:29:28Z

Yup. I was working on it a bit yesterday. It's been a busy week, but I should have something out by Friday. I'll open this as a new PR.

nfischer · 2016-03-05T03:02:49Z

I'm working on refactoring this today. I should have this out by tonight in a new PR.

levithomason · 2016-03-14T15:27:03Z

Bump, this PR is tagged as blocking 1st release of shx. Anything I can do to help?

nfischer · 2016-03-14T17:46:58Z

I'll wrap this up later this week, just a little in over my head right now.

@levithomason could you investigate a fix for how shelljs commands (that have fatal errors) don't output to the console sometimes? the issue was introduced when we converted config.fatal to throw an exception

levithomason · 2016-03-14T17:52:19Z

Sure thing, I'll review this issue and see what I can find.

nfischer · 2016-03-14T22:18:22Z

@levithomason I'll try to make a bug report that more clearly documents the problem.

And I'll get out a primitive implementation of error codes today (finally got some spare time).

levithomason · 2016-03-15T14:39:29Z

Sounds good, I pulled this branch and perused around but needed more context. Will wait to hear back from you.

nfischer · 2016-03-15T23:54:06Z

@levithomason the bug isn't in this branch. This branch is pretty ancient at this point (I'm refactoring everything in a new branch).

I'll ping you in the bug report, and describe it better there.

nfischer · 2016-03-16T02:48:58Z

I'm closing this, since #394 is a better replacement

nfischer added feature fix Bug/defect, or a fix for such a problem medium priority labels Jan 11, 2016

nfischer assigned ariporad Jan 11, 2016

ariporad reviewed Jan 11, 2016
View reviewed changes

ariporad mentioned this pull request Jan 24, 2016

error() is broken after running exec() #156

Closed

nfischer mentioned this pull request Jan 24, 2016

Add new CLI script for running commands #244

Closed

feat(errorCodes): implement error codes

12815e7

Implement error codes internally. The script will exit with the status of the last command. The default code for errors is 1.

ariporad added this to the v0.6.0 milestone Jan 31, 2016

nfischer modified the milestones: v0.7.0, v0.6.0 Feb 2, 2016

ariporad mentioned this pull request Feb 15, 2016

Standardize command output #356

Closed

nfischer mentioned this pull request Feb 16, 2016

Initial Discussion and TODOs for 1st release shelljs/shx#1

Closed

7 tasks

ariporad added high priority and removed medium priority labels Feb 16, 2016

nfischer added the blocked label Feb 19, 2016

nfischer mentioned this pull request Feb 20, 2016

Refactor shellstring #360

Merged

nfischer added medium priority and removed fix Bug/defect, or a fix for such a problem high priority labels Feb 20, 2016

nfischer mentioned this pull request Mar 16, 2016

feat: adding error codes to ShellJS #394

Merged

nfischer closed this Mar 16, 2016

Implementing error codes and adding the errorCode() function #269

Implementing error codes and adding the errorCode() function #269

Uh oh!

Conversation

nfischer commented Dec 30, 2015

Uh oh!

nfischer commented Jan 11, 2016

Uh oh!

ariporad Jan 11, 2016

Choose a reason for hiding this comment

Uh oh!

nfischer Jan 11, 2016

Choose a reason for hiding this comment

Uh oh!

ariporad commented Jan 11, 2016

Uh oh!

nfischer commented Jan 11, 2016

Uh oh!

nfischer commented Jan 11, 2016

Uh oh!

nfischer commented Jan 15, 2016

Uh oh!

ariporad commented Jan 17, 2016

Uh oh!

nfischer commented Jan 17, 2016

Uh oh!

ariporad commented Jan 17, 2016

Uh oh!

nfischer commented Jan 18, 2016

Uh oh!

nfischer commented Jan 24, 2016

Uh oh!

ariporad commented Jan 28, 2016

Uh oh!

nfischer commented Jan 28, 2016

Uh oh!

ariporad commented Jan 31, 2016

Uh oh!

nfischer commented Jan 31, 2016

Uh oh!

ariporad commented Feb 14, 2016

Uh oh!

nfischer commented Feb 15, 2016

Uh oh!

ariporad commented Feb 19, 2016

Uh oh!

nfischer commented Feb 19, 2016

Uh oh!

ariporad commented Feb 24, 2016

Uh oh!

nfischer commented Feb 24, 2016

Uh oh!

nfischer commented Mar 5, 2016

Uh oh!

levithomason commented Mar 14, 2016

Uh oh!

nfischer commented Mar 14, 2016

Uh oh!

levithomason commented Mar 14, 2016

Uh oh!

nfischer commented Mar 14, 2016

Uh oh!

levithomason commented Mar 15, 2016

Uh oh!

nfischer commented Mar 15, 2016

Uh oh!

nfischer commented Mar 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants