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

Fixes #20

Closed
wants to merge 4 commits into from
Closed

Fixes #20

wants to merge 4 commits into from

Conversation

galkin
Copy link
Contributor

@galkin galkin commented Jan 2, 2017

Copy link
Owner

@rolodato rolodato left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR. I can't merge this all at once, but I'll create a new release to fix the immediate issue. If you still want to add linting and improve the tests after that, much appreciated!

@@ -2,56 +2,80 @@

var dotenv = require('dotenv');
var fs = require('fs');
var MissingEnvVarsError = require('./MissingEnvVarsError.js');
var util = require('util');
function MissingEnvVarsError(allowEmptyValues, dotenvFilename, sampleFilename, missingVars, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

This error type should be in its own file. We can solve this by adding "MissingEnvVarsError.js" to the files array of package.json.

'no-var': 0,
'max-len': [2, {
code: 120,
tabWidth: 2,
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, 4 space tab width is intended here :)

Please use the following settings which also avoid an eslint plugin install (2 errors about comma-dangle in current master commit):

module.exports = {
    "extends": "eslint:recommended",
    "env": {
        "node": true,
	"mocha": true
    },
    "rules": {
        "no-trailing-spaces": "error",
        "semi": "error",
        "indent": ["error", 4],
	"comma-dangle": ["error", "never"]
    }
};

"test": "mocha"
},
"dependencies": {
"dotenv": "github:motdotla/dotenv#fdd0923e82e12a6e29b65898990201857141e75d"
},
"devDependencies": {
"chai": "^3.5.0",
"eslint": "^3.12.2",
"eslint-config-google": "^0.7.1",
Copy link
Owner

Choose a reason for hiding this comment

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

This should be removed in favor of eslint:recommended


[*]
indent_style = space
indent_size = 2
Copy link
Owner

Choose a reason for hiding this comment

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

Please update .editorconfig to match comment in .eslintrc.js.

@@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [4.0.1] - 2017-01-03
### Changed
- Move missed MissingEnvVarsError function to index file
Copy link
Owner

Choose a reason for hiding this comment

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

This should be:

### Fixed
- Fix missing `MissingEnvVarsError.js` error when calling `load()`

## [4.0.1] - 2017-01-03
### Changed
- Move missed MissingEnvVarsError function to index file
- Fix inconsistent tests
Copy link
Owner

Choose a reason for hiding this comment

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

Not user facing, shouldn't be in changelog.

### Changed
- Move missed MissingEnvVarsError function to index file
- Fix inconsistent tests
- Add eslint for code style checks
Copy link
Owner

Choose a reason for hiding this comment

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

Not user facing, shouldn't be in changelog.

"keywords": [
"dotenv"
],
"license": "MIT",
"author": "Rodrigo López Dato <rlopezdato@gmail.com>",
"files": [
"index.js",
"config.js"
Copy link
Owner

Choose a reason for hiding this comment

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

"MissingEnvVarsError.js" should be added here

@galkin
Copy link
Contributor Author

galkin commented Jan 3, 2017

Closed as outdated.

@galkin galkin closed this Jan 3, 2017
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.

2 participants