Skip to content

Refactor #57

Merged
merged 20 commits into from Jan 14, 2013

4 participants

@fb55
fb55 commented Sep 8, 2012

As you might have seen, I did some refactoring in the refactor branch. Mainly removing unused code, improving and abstracting some stuff and introducing two new public methods:

  • analyzer.path does ra's magic on a path, no matter what kind of file is behind it.
  • analyzer.findNextDir does what .findModulesDir did so long: Return the next directory for a path.

.findModulesDir now actually searches for a directory containing either a node_modules dir or a package.json file.

findit is no longer a dependency, as .path (which is an abstraction of .dir) can do the job.

Please have a look at the code, it's possible that I've missed something. (All tests pass at least.)

fb55 added some commits Sep 7, 2012
@fb55 fb55 use console.log instead of util.puts
also fixed some linting errors (semicolons & equals)
c802fea
@fb55 fb55 added a analyzer.path method that calls the appropriate function for …
…a path

as it's mainly the code of .analyze, use it there
5d32a73
@fb55 fb55 use .path inside .dir (replaces findit) e432496
@fb55 fb55 removed findit from dependencies b12e72d
@fb55 fb55 some minor changes
• use the `in` operator more frequently (it's much nicer than `typeof
foo !== "undefined"` + is more readable)
• removed some garbage
• added some comments
f74cfd3
@fb55 fb55 removed two unnecessary functions
in L54, err would have always be falsey (or the function had returned
earlier)
e1aa6b0
@fb55 fb55 [fix] still accessed property before checking it with `in` f08e5fc
@fb55 fb55 [fix] passed filenames instead of paths to .path 55faa29
@fb55 fb55 forward errors from prerunners in runAnalyze a7c35d1
@fb55 fb55 use Function#bind when it's appropriate, use single quotes d9c45eb
@fb55 fb55 Revert "forward errors from prerunners in runAnalyze"
This reverts commit a7c35d1.
48a34e0
@fb55 fb55 always return an object in npmAnalyze
also moved the creation of the find-dependencies path to the
surrounding scope
7ede462
@fb55 fb55 [fix] move upwards in findModulesDir
this is the functionality that is described in the comments, but wasn't
implemented (yet)
b3e2bb9
@fb55 fb55 removed senseless check in analyzer.dir
a folder should never contain a file that has the folder's path as it's
name
49b2d44
@fb55 fb55 added analyzer.findNextDir function
the last change broke most tests, so .findNextDir is what's actually
wanted for .npmAnalyze
16f22f9
@fb55 fb55 set 1 as the maxDepth of read-installed
we don't use more, so don't do anything fancy
eeebe3a
@fb55 fb55 [fix] call callback when a node_modules folder is present 873a641
@fb55 fb55 simplified mergeDependencies & .package
• removed scanning scripts as they weren't implemented properly (they
just looped a couple of times without doing anything)
• simplified signature of mergeDependencies
• return errors in .package
• also updated some comments
e169c9e
@fb55 fb55 removed some garbage in .package 4968886
@fb55 fb55 [fix] call .findNextDir recursively 2352c31
@indexzero
nodejitsu member

@mmalecki @AvianFlu We should look at this in the next week or two to evaluate it. No rush though.

@dscape dscape commented on the diff Dec 28, 2012
lib/require-analyzer.js
if (!options || !options.target) {
//
// If there are no `options` and no `options.target` property
// respond with the appropriate error.
//
callback(new Error('options and options.target are required'));
- return emitter;
@dscape
nodejitsu member
dscape added a note Dec 28, 2012

Needs review. Is this depended upon anywhere?

Guessing not, as in the error we don't return any ee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dscape
nodejitsu member
dscape commented Dec 28, 2012

LGTM. Seems like great work by @fb55

Tests pass locally

@yawnt yawnt merged commit 6ece1ff into master Jan 14, 2013

1 check passed

Details default The Travis build passed
@fb55 fb55 deleted the refactor branch Jan 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.