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

Updated README.md and added mocha to run the existing test. #1

Closed
wants to merge 3 commits into from
Closed

Updated README.md and added mocha to run the existing test. #1

wants to merge 3 commits into from

Conversation

tydira
Copy link

@tydira tydira commented Dec 27, 2016

When I imported 'iterate' from the package, the value was undefined. I realized that the README.md was misleading, so this PR adjusts it to reflect what the package exports ('iteration').

I'm having another issue with the library, which might end up in another PR, so I wanted to run the test suite to see what happens and realized that it didn't come with mocha and needed an improved test command (both using mocha and not relying on any local PATH modifications).

@@ -22,19 +22,19 @@ As soon as you keep a reference to an object, array, arrow function, ... you do

```js
import myLib from 'my-lib'
import { iterate } from 'leakage'
import { iteration } from 'leakage'
Copy link

Choose a reason for hiding this comment

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

Thanks for fixing this! It took me a moment to figure out why my import wasn't working before I discovered the name was different.

@@ -4,7 +4,7 @@
"description": "Memory leak testing for node. Write tests using your favorite test runner.",
"main": "lib/index.js",
"scripts": {
"test": "standard lib/**/*.js"
"test": "node_modules/.bin/standard lib/**/*.js && node_modules/.bin/mocha test"
Copy link

Choose a reason for hiding this comment

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

npm adds node_modules/.bin to PATH, so you can use standard and mocha without the manual path prefix

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I didn't know they added this. Thank you.

Copy link

Choose a reason for hiding this comment

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

👍 Sure thing! Here's that section in the docs, in case you're interested in reading more: https://docs.npmjs.com/misc/scripts#path

@andywer
Copy link
Owner

andywer commented Dec 28, 2016

Ahhh, I'm sorry for the misleading docs, guys! Thanks for fixing, @kroogs!

Copy link
Owner

@andywer andywer left a comment

Choose a reason for hiding this comment

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

Just one thing: Now you run mocha on npm test (see) package.json. But the test in test/ is supposed to fail, so npm test will always fail.

Let's skip the package.json modification for now. I will open a new PR introducing at least a little bit of unit/integration testing.

This was referenced Dec 28, 2016
@andywer
Copy link
Owner

andywer commented Dec 28, 2016

@btmills @kroogs Do you prefer iterate or iteration?

@btmills
Copy link

btmills commented Dec 28, 2016

iterate feels more natural to me. And if you want to avoid a breaking change, you can rename it and change the docs, then do module.exports = { iterate, iteration: iterate };, while officially deprecating iteration.

@andywer
Copy link
Owner

andywer commented Dec 28, 2016

@btmills Yeah, feeling the same. I will just edit the PR.

@andywer
Copy link
Owner

andywer commented Dec 28, 2016

Or maybe not... Guess it's easier to start a new one, since editing so much isn't easy using the GitHub web UI.

@andywer
Copy link
Owner

andywer commented Dec 28, 2016

Opened a new PR. Still thankful that you opened this one, @kroogs :)

@andywer andywer closed this Dec 28, 2016
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

3 participants