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

Refactoring getOptions #23

Merged
merged 3 commits into from
Sep 7, 2013
Merged

Refactoring getOptions #23

merged 3 commits into from
Sep 7, 2013

Conversation

mattjmorrison
Copy link
Contributor

Here is an example of something that I think would work for the grunt-karma-coveralls plugin for issue #22.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 3ab2c57 on mattjmorrison:master into 167119f on cainus:master.

@cainus
Copy link
Collaborator

cainus commented Sep 7, 2013

Yep that's fine (theoretically). I suspect that the way you did the export won't get you the result that you're looking for though (You're not actually exporting getBaseOptions()). If you add a simple test that uses getBaseOptions() directly, you'll see what I mean (we should probably have a test on that anyway, since this is a coverage tool.).

Also updated index.js to prevent any breaking API changes and added
tests around both getOptions and getBaseOptions.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 8078aa0 on mattjmorrison:master into 167119f on cainus:master.

@mattjmorrison
Copy link
Contributor Author

Good call on the exports, I totally added that as an after thought. I refactored the tests so the same tests will hit both getOptions and getBaseOptions and I also updated index.js so that getOptions should behave the same as it did before.

@cainus
Copy link
Collaborator

cainus commented Sep 7, 2013

Ha very nice... I'm re-running that one build and expect it to pass. Sometimes coveralls.io gets flakey when I hit it with multiple coverage reports at once.

One question though: Don't you want index.js to export getBaseOptions?

@mattjmorrison
Copy link
Contributor Author

If you would like, I can add getBaseOptions to index.js. I was planning on just doing something like this

var getBaseOptions = require('../lib/getOptions').getBaseOptions;

Adding it to index.js would probably be cleaner. I will go ahead and add that and update the tests to reflect that change.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 2d4e059 on mattjmorrison:master into 167119f on cainus:master.

cainus added a commit that referenced this pull request Sep 7, 2013
@cainus cainus merged commit 6c5d9df into nickmerwin:master Sep 7, 2013
@mattjmorrison
Copy link
Contributor Author

Thanks. Do you know when you are going to publish a new version of node-coveralls to npm?

@cainus
Copy link
Collaborator

cainus commented Sep 7, 2013

Just did! It's 2.3.0. Great work! Thanks for the help. Let me know if you need anything else.

@mattjmorrison mattjmorrison mentioned this pull request Sep 7, 2013
@mattjmorrison
Copy link
Contributor Author

@cainus did you publish to npm? The latest version that I see out there is 2.2.0.

@cainus
Copy link
Collaborator

cainus commented Sep 7, 2013

Bah sorry... guess I skipped that step... just did it though.

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