Build fail issues

[andrewnicols]

This addresses the various build fail issues that were raised in #115, #118 and #120.

Previous shifter behaviour was broken in a few places and a lot of failures were not printing the errors correctly. This patchset addresses those issues.

This patchset primarily:

• stops printing all errors at the end of a run (as per previous behaviour);
• fails as soon as a build fail occurs
• fails on some conditions which were incorrectly not failing before (empty skins)
• actually displays an error when the build fails rather than the standard 'done' message - hopefully this will reduce confusion

It additionally:

• doesn't fail if the source file was also empty; and
• changes lint failures to warnings since they don't actually break a build.

I haven't been able to address the unit test failures yet - if someone else has time to do so that'd be good. For some reason I'm not seeing all of the unit tests running when run through npm test, and I'm not getting a list of the failures in order to fix them.

I have, however, built YUI3, and Moodle and am not seeing any shift failures on either project. I've also tested that:

• failures do happen if they should, e.g. with:
  • invalid files in the jsfiles listing
  • missing skin files
  • invalid build directories (no build.json and no properties.

[andrewnicols]

Worked out what was going on with the tests and now everything is passing once more.

Hope that this is all good for you @caridy. IMO, these changes make the failures clearer and I think that they do the right thing WRT callbacks too.

[caridy]

if you're about to hit the brakes, why logging the error? normally the error will be logged in the callback

[caridy]

@andrewnicols thanks for taking the time to dig on this, much appreciated.

for the sake of simplicity (I already had a hard time dealing with the situation), let's try to keep the changes to the minimum until we release the next version.

my main concern at this point is the different workflow, In PR #120 we agreed to have a callback taking care of logging the error and propagating the error upstream to the parent without having to log the error everywhere in the codebase, I will like to stick to that principle.

[andrewnicols]

Hi @caridy,

Thanks for taking the time to look at and review this so quickly.

When I first started looking at this, I found that many of the errors which are thrown are inconsistent in format. From what I can tell, the callback work was not completed. As an example, you'll see at https://github.com/yui/shifter/blob/master/lib/module.js#L364 that the callback is called with err, but err is an object and the default callback does not handle that properly. That's why we're getting that nasty output. So in that example (on master), there's a combination of both log.err(err), and callback(err) - but neither works at all.

IMO, in order for a callback to be actually useful, we must standardise the format that content passed into it follows, otherwise it's just a meaningless jumble of different types of object which some poor soul has the misfortune to work with. At present we do not do this.

In my example above, this occasion of err is assumed to be a string for the purposes of the regex tests (366 and 368), but then on line 369 it's assumed to be an object with structure:

err: {
  path: 'Some path'
}

In actual fact, and from what I can tell, it is actually an object with a structure:

err: {
  task: 'check',
  error: {
    name: 'somename',
    error: 'foo'
  }
}

As a result, this particular example of the err object always hits the else case which just tries to log err and we get something like:

shifter [err] widget-stack: [object Object]

(this example can be reached on any version of shifter, including current master, by specifying a jsfile which does not exist)

Only, sometimes it does does not fit the structure above, sometimes we get:

err: {
  error: 'Some error string'
}

This is the case for any fail of the .check() function - for example in the error which is currently ignored at https://github.com/yui/shifter/blob/master/lib/module.js#L570 when we have skin files which are empty.

In order to get things working in a sane and usable fashion for the moment, and since I work opposite hours to you and was unable to ask, I went for the option of making the logging work but keeping much of the current callback behaviour consistent. The alternative would be to put a load of logic into the standard callback (logAndExit), but to me this seems wrong. We should not expect anyone using the code to have to do this same calculation to catch every weird and wonderful combination we apparently support.

If you want to keep with the logAndExit work, then this whole callback stuff needs finishing:

• removing all places where we call log.err and should be using a callback;
• standardising the callback data structure; and
• fixing the default callback to actually show the errors when they are encountered.

The callback also only handles log.err, so it's unclear what should happen for info and warnings. Surely the callback should be handling these as the parent app may care about them. Equally, because it currently exits if a message was specified, it's impossible to log multiple lines.

IMHO, I think that all of the current callback changes should be reverted to the state at which they were in 0.4.6, and a new version released now. The callbacks work isn't there yet and, as I say, I think that the arguments to the callback need to be better defined.

I'm happy to help get this going further, but I don't think that I have enough time to work on standardising the calls to callback right now.

Hope that this helps,

Andrew

[andrewnicols]

Hi @caridy,

If you have some time, it'd be really good to try and work out the best way to progress this so we can get a new version of Shifter released. What are the best times to get hold of you on hangouts?

Cheers,

Andrew