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

Added options.node_env that refers process.env.NODE_ENV by default #6

Merged
merged 18 commits into from
Dec 15, 2018

Conversation

MasonRhodesDev
Copy link
Contributor

@MasonRhodesDev MasonRhodesDev commented Dec 13, 2018

I went ahead and put together the changes we talked about in issue 5.

If this is up to standards and keeps within the goals of this module please consider reviewing.

Thanks!

Copy link
Owner

@kerimdzhanov kerimdzhanov left a comment

Choose a reason for hiding this comment

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

Also, don't forget to squash your commits into a single one, and please follow the angular commit message conventions, because we use conventional-changelog to generate the CHANGELOG.md when releasing a new versions. Also you can refer the issue #5 in your commit message.

Summarizing the above, I want this PR to be a single commit with message like:

feat(options): add `options.node_env` implementation, closes #5

lib/dotenv-flow.js Outdated Show resolved Hide resolved
lib/dotenv-flow.js Show resolved Hide resolved
lib/dotenv-flow.js Outdated Show resolved Hide resolved
@kerimdzhanov kerimdzhanov changed the title Added options.node_env as override Added options.node_env that refers process.env.NODE_ENV by default Dec 13, 2018
lib/dotenv-flow.js Outdated Show resolved Hide resolved
@MasonRhodesDev
Copy link
Contributor Author

I've made all the changes we talked about above. But in the process we did end up removing the line that sets process.env.NODE_ENV to options.default_node_env so this
```describe('when the default_node_env option is provided', () => {````
test fails now.

Should I replace the setter that we removed or leave setting NODE_ENV up to the user?

@kerimdzhanov
Copy link
Owner

kerimdzhanov commented Dec 14, 2018

Test shouldn't be changed on removing process.env.NODE_ENV, sorry, I tough it's obvious :)
We just shouldn't refer process.env.NODE_ENV nowhere except our one-liner, the idea is to be totally independent from process.env.NODE_ENV when we provide our custom node_env as an option. In other words, we shouldn't touch the variable at all, like it doesn't exists.

@MasonRhodesDev
Copy link
Contributor Author

Un-related to the code, but what would be the best option for addressing the original issue you presented? to follow angular commit standards would the best option after the other issues are fixed to re-submit a new pull request or is there a way to address that here? (to be honest I don't have much experience contributing to other's projects on github and I really appreciate your patience with me thus far)

Co-Authored-By: solaris765 <bassvaulter@gmail.com>
@kerimdzhanov
Copy link
Owner

You can try to git squash then push --force to the same branch, I'm not sure does it work with github, but with bitbucket it works.

@MasonRhodesDev
Copy link
Contributor Author

ok, I'll try that after I finish

@kerimdzhanov
Copy link
Owner

Or maybe it would be better to try squash merge PR with github. Leave it as is for now.

@kerimdzhanov
Copy link
Owner

Yes, exactly, we can do squash merge 👍

@MasonRhodesDev
Copy link
Contributor Author

MasonRhodesDev commented Dec 14, 2018

ok, I think its ready for review number 2.

(its also getting late for me, so if there are anymore changes that need to be made I'll probably address them tomorrow)

@kerimdzhanov
Copy link
Owner

Everything else looks perfect 👍

lib/dotenv-flow.js Outdated Show resolved Hide resolved
kerimdzhanov and others added 2 commits December 14, 2018 14:54
Co-Authored-By: solaris765 <bassvaulter@gmail.com>
@kerimdzhanov kerimdzhanov merged commit 30db488 into kerimdzhanov:master Dec 15, 2018
@kerimdzhanov
Copy link
Owner

kerimdzhanov commented Dec 15, 2018

@solaris765, good job 👍
You are the first contributor of the project, thank you! 🍻
Don't forget to ⭐️ the project you've contributed to ;)

@MasonRhodesDev
Copy link
Contributor Author

Awesome! Done!

kerimdzhanov added a commit that referenced this pull request Dec 16, 2018
Add documentation for the `node_env` initialization option implemented in PR #6
@kerimdzhanov
Copy link
Owner

@solaris765, I've released a new candidate v0.2.0-rc.1. You can try it.

$ npm install "dotenv-flow@latest"

or

$ yarn add dotenv-flow

Let me know if you find any related bugs.

@MasonRhodesDev
Copy link
Contributor Author

Got it. I'll try it now. I have a use case that's currently referencing an earlier version of my git fork that I can move over for testing

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