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

Add support for .env files #3

Merged
merged 5 commits into from
Aug 23, 2017
Merged

Conversation

dexafree
Copy link
Contributor

Fixes #2

envy.go Outdated

// It exists and we have permission. Load it
err = godotenv.Overload(file)
Reload()
Copy link
Member

Choose a reason for hiding this comment

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

The call to Reload should probably get moved to after the err check. There's no point in reloading if there was an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right. It has been modified now :)

Copy link
Member

@markbates markbates left a comment

Choose a reason for hiding this comment

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

A couple more changes, sorry. Also, I wonder if it makes sense to try and load .env.XX based on the GO_ENV ENV var. Buffalo uses GO_ENV to switch to production, development, etc... it would be nice if these files were "auto-loaded", if they exist.

envy.go Outdated
// If no files received, load the default one
if len(files) == 0 {
err := godotenv.Overload()
Reload()
Copy link
Member

Choose a reason for hiding this comment

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

There's no error check here. Can we add one:

if err := godotenv.Overload(); err == nil {
  Reload()
}
return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we wouldn't be returning the error.

We could change it to

err := godotenv.Overload()
if err == nil {
  Reload()
}
return err

to avoid the unnecessary Reload call (and indeed returning the error)

Copy link
Member

Choose a reason for hiding this comment

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

that seems so weird, but yeah, let's do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 73986d1

envy.go Outdated
for _, file := range files {

// Check if it exists or we can access
_, err := os.Stat(file)
Copy link
Member

Choose a reason for hiding this comment

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

can we collapse these error checks in this for block to use the if err := dosomething; err != nil style syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 094661c

envy.go Outdated
// If no files received, load the default one
if len(files) == 0 {
err := godotenv.Overload()
Reload()
Copy link
Member

Choose a reason for hiding this comment

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

that seems so weird, but yeah, let's do that.

@dexafree
Copy link
Contributor Author

Also, I wonder if it makes sense to try and load .env.XX based on the GO_ENV ENV var. Buffalo uses GO_ENV to switch to production, development, etc... it would be nice if these files were "auto-loaded", if they exist.

That change would be added in the Buffalo project, not here. But first I needed to add support for dotenv files in envy, and then I would be able to add this "autoload" to buffalo.

Note that it wouldn't be useful in things such as:

const PORT = envy.Get("PORT", "8080")

As this kind of values are resolved before any other call has been made (including the envy.Reload one).
But this cannot be solved by any means with any library, as it's a language behaviour

@markbates markbates merged commit 45f72f3 into gobuffalo:master Aug 23, 2017
@markbates
Copy link
Member

thanks!

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

2 participants