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.

Sign up for GitHub

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

Jump to bottom

Update Module with Tests and 100% Test Coverage #8

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Update Module with Tests and 100% Test Coverage #8

wants to merge 9 commits into from

Conversation

nelsonic
Copy link
Collaborator

Hi @dshaw,
As promised: #6 here's the Pull Request with the update to latest node.js (apologies for taking so long, we got "distracted" with our work... 😉 )

Build Status Test Coverage Code Climate Dependency Status devDependency Status

What's included in the PR?

  • migrated tests from Tap to Tape so that we could collect code coverage info with Istanbul.
  • added istanbul for code coverage (100%)
  • sends code coverage stats to codeclimate.com (which also tracks code quality)
  • incremented the version number to 1.0.0 to be Semver-compliant
  • removed one line of unused/untested code from lib/env.js
  • but, did not change any functionality in your module (so it should be 100% backward compatible)
  • added pre-commit hook to check coverage so future contributions do not reduce test covreage
  • added a couple more badges to the readme so that its clear to people that this module is maintained/loved! (the links are to our travis/codeclimate/dependencies we intend to keep the fork in our org for reference but the readme links can easily be changed with a find-and-replace if you like)

Please let us know if this is ok. otherwise let us know what you would prefer us to change.

Thanks!
N 🚀

@nelsonic nelsonic mentioned this pull request Jul 17, 2015
Open
nelsonic
nelsonic reviewed Jul 17, 2015
View reviewed changes
lib/env.js
this.envfile = path.join(process.cwd(), envfile || 'env.json')
this.evars = (path.existsSync(this.envfile)) ? require(this.envfile) : {}
this.envfile = envfile || path.resolve(process.cwd() + '/env.json');
this.evars = (fs.statSync(this.envfile)) && require(this.envfile)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We use fs.statSync because if the env.json file does not exist most apps probably wont work to the error (with full stack trace) is appropriate. what do you think?

@nelsonic
Copy link
Collaborator Author

@dshaw please let us know if anything is missing to get this PR merged.
I promise I won't be offended if you tell me to rip out chunks of it because I've gone too far or something...
We just really want to use your env module in our app(s) but can't until this update goes in ...
thanks!

@JoseCage
Copy link

Why this PR stills Open?? (not merged 🐛 🐛 )

@nelsonic nelsonic mentioned this pull request Jul 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nelsonic @JoseCage