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

Load ENV via dotenv if available #53

Merged
merged 1 commit into from
Apr 14, 2016
Merged

Conversation

Crunch09
Copy link
Member

Hi,
i didn't know if tests are needed for this because i think they would only test the behavior already tested in the dotenv repository. Also i'm not sure if i should add something to the README about this.

@@ -1,4 +1,9 @@
require 'octokit'
begin
require 'dotenv'
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about wrapping these two lines (also) in a if Jekyll.env == 'development'?

I believe we'll also need to require 'jekyll' then, which I don't think will cause any problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, sounds good! But i think we should put require 'jekyll' also in the begin block then because it was removed in 80370fc and it's only a development dependency in the gemspec

Copy link
Contributor

Choose a reason for hiding this comment

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

The other way may be:

if defined? Jekyll && Jekyll.env == "development"

@Crunch09
Copy link
Member Author

Crunch09 commented Apr 2, 2016

@benbalter Hi, i updated the PR as we discussed.

@benbalter
Copy link
Contributor

@parkr what do you think?

@parkr
Copy link
Member

parkr commented Apr 14, 2016

@benbalter I THINK GOOOOOOD!

@benbalter
Copy link
Contributor

Thanks @Crunch09! 🌷 🐢 🎉

@benbalter benbalter merged commit 4a4dad2 into jekyll:master Apr 14, 2016
@jekyll jekyll locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants