Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#33] Search for packages locally #34

Merged
merged 5 commits into from
Sep 1, 2015
Merged

[#33] Search for packages locally #34

merged 5 commits into from
Sep 1, 2015

Conversation

nkbt
Copy link
Contributor

@nkbt nkbt commented Aug 25, 2015

  1. Added ability to search for global and local packages
  2. Updated tests
  3. Fixed issue with incorrect packages (without name), thanks to additional test
  4. Updated cli to merge local and global packages
  5. Fixed build command to require correct package CLI

Tested by installing fixed (corrected main) cf-package from fs: npm install ~/nkbt/cf-package. All picked up:

20150825-232649

@nkbt nkbt mentioned this pull request Aug 25, 2015
@@ -58,22 +63,52 @@ const after = () => {
};


test('Find Scaffolds success', assert => {
test('Find Scaffolds API', assert => {
before();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: instead of using a generic name like this, you could use a more semantic name like "stubNpm()".

Also, as a matter of discipline, you could also localize the npm var and inject it as a dependency to findGlobal rather than requiring it. That way you separate the side-effects (npm API I/O) from the program logic, and it allows you to do this:

const npm = stubNpm();

// ... do stuff

resetNpm(npm);
assert.end();

I find this easier to read because you can see what's going on inline without scrolling up and looking at the before() and after() functions. You also avoid the common situation where some tests need more setup/teardown than others, but you run everything for all tests. For example, your first test doesn't need the stub at all (unless I'm reading it wrong), but you're calling the functions anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that all makes a lot of sense. After I started with tape I began to "purify" my functions and modules as much as possible. Will update this code as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 On a related note... I'm going into lots of detail about how to write better tests in a webcast next week.

@nkbt
Copy link
Contributor Author

nkbt commented Aug 29, 2015

I've made a little refactoring to keep find-scaffolds pure by passing npm in. Which simplified tests.

No need to reset npm mock, since it is always freshly created.

new Promise((...args) => npm.load({global, silent: true, depth: 0}, loaded(npm)(...args)));


export const findLocal = npm => findScaffolds(npm)(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill the boolean trap, please. Switch on 'local'/'global' instead of true/false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice article, thanks.

@ericelliott
Copy link
Contributor

lgtm... remove the boolean trap, and I think we're good to go, right?

@nkbt
Copy link
Contributor Author

nkbt commented Sep 1, 2015 via email

@ericelliott
Copy link
Contributor

👍

@nkbt
Copy link
Contributor Author

nkbt commented Sep 1, 2015

Pushed the code for boolean trap fix.

new Promise((...args) => npm.load({silent: true, depth: 0, ...options}, loaded(npm)(...args)));


export const findLocal = npm => findScaffolds(npm)({global: false});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice: =)

ericelliott pushed a commit that referenced this pull request Sep 1, 2015
[#33] Search for packages locally
@ericelliott ericelliott merged commit df7e3d8 into cloverfield-tools:master Sep 1, 2015
@nkbt nkbt deleted the find-local branch September 1, 2015 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants