Clean up API #1061
Clean up API #1061
Conversation
vadimdemedes
force-pushed
the
cleanup-api
branch
4 times, most recently
from
93d1b9b to
ea25e80
Compare
|
PR is ready for review! |
There was a problem hiding this comment.
Generally looks good, but a little bit hard to review due to all the changes. I ended up looking at https://github.com/avajs/ava/blob/ea25e802bce67b195d5f0fc331291bb02f16b866/api.js and https://github.com/avajs/ava/blob/master/api.js on a per method basis.
| var findOptions = { | ||
| cwd: this.options.resolveTestsFrom, | ||
| files: files | ||
| }; |
There was a problem hiding this comment.
I don't really see how this improves anything though. It was already clear what the options are for.
There was a problem hiding this comment.
The point of this is not the name (I named it findOptions, because options is already defined there), but extracting options to reduce nesting. I can move it back, not insisting on this one.
| options = {}; | ||
| } | ||
|
|
||
| var prefixTitles = this.options.explicitTitles || files.length > 1; |
There was a problem hiding this comment.
I don't see the point of extracting this either. It has the same title as the option name, so I don't see how this makes it any clearer.
| Api.prototype._run = function (files, options) { | ||
| if (!options) { | ||
| options = {}; | ||
| } |
| @@ -153,7 +153,14 @@ module.exports = function (file, opts, execArgv) { | |||
| return promise; | |||
| }; | |||
|
|
|||
| var isExitInitiated = false; | |||
There was a problem hiding this comment.
What change made this necessary? Maybe worth using onetime here?
There was a problem hiding this comment.
Unfortunately, I forgot... I remember it had something to do with this
Line 305 in ea25e80
onetime.
There was a problem hiding this comment.
I removed this completely and tests ran successfully, I guess there's no need for it.
|
|
||
| runStatus.processResults(results); | ||
|
|
||
| return runStatus; | ||
| }); | ||
| }; | ||
|
|
||
| function getBlankResults() { |
There was a problem hiding this comment.
Can you move this to the the top? Better to have it defined before use.
| base: path.relative('.', commonPathPrefix(files)) + path.sep | ||
| runOnlyExclusive: options.runOnlyExclusive, | ||
| prefixTitles: prefixTitles, | ||
| base: basePath |
There was a problem hiding this comment.
If base is unclear, I think we instead should rename it in RunStatus.
There was a problem hiding this comment.
Yes, it'd rename it to basePath, but in a separate PR dedicated to cleaning up RunStatus.
|
Would be good if @jamestalmage could review, as he's the one that wrote most of the code in |
vadimdemedes
force-pushed
the
cleanup-api
branch
from
ea25e80 to
e3fcc4e
Compare
|
@sindresorhus addressed all your feedback, thanks! |
vadimdemedes
force-pushed
the
cleanup-api
branch
from
e3fcc4e to
86fe0e2
Compare
|
LGTM as well :-) Good work! |
|
Alright. Landed. Good stuff @vdemedes :) |
|
Thanks guys! |
Starting a series of "cleaning up" PRs to make AVA's codebase cleaner and friendlier to everyone. These PRs don't change any functionality and don't modify tests on purpose, those are just code improvements.
This PR removes lots of nesting, nested functions (
tryRun(),run(),runner()) and confusing code. Also, it brings little improvements in the naming, options objects and makes functions shorter by extracting their parts into individual functions.