Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Clean up duplication in #load, #load! and #overload #96

Merged
merged 2 commits into from Mar 8, 2014

Conversation

Projects
None yet
3 participants
Owner

bkeepers commented Feb 23, 2014

Just a small refactoring to remove some code that has always bothered me.

@elskwid elskwid commented on the diff Feb 23, 2014

spec/dotenv_spec.rb
@@ -72,22 +72,21 @@
context 'when one file exists and one does not' do
let(:env_files) { ['.env', '.env_does_not_exist'] }
- it 'raises an Errno::ENOENT error and does not load any files' do
- expect do
- expect do
- subject
- end.to raise_error(Errno::ENOENT)
- end.to_not change { ENV.keys }
+ it 'raises an Errno::ENOENT error' do
@elskwid

elskwid Feb 23, 2014

So much better.

@elskwid elskwid commented on the diff Feb 23, 2014

lib/dotenv.rb
end
# same as `load`, but raises Errno::ENOENT if any files don't exist
def self.load!(*filenames)
- load(
- *default_if_empty(filenames).each do |filename|
- filename = File.expand_path filename
- raise(Errno::ENOENT.new(filename)) unless File.exists?(filename)
@elskwid

elskwid Feb 23, 2014

I like the removal of this and instead having the .read method in Environment just error out. API is still compatible but the code in dotenv.rb is much cleaner.

elskwid commented Feb 23, 2014

I'd merge this if I could. 💯

@parkr parkr and 1 other commented on an outdated diff Feb 23, 2014

lib/dotenv.rb
end
protected
- def self.default_if_empty(filenames)
- filenames.empty? ? (filenames << '.env') : filenames
+
+ def self.with(*filenames, &block)
+ filenames << '.env' if filenames.empty?
+
+ filenames.inject({}) do |hash, filename|
+ filename = File.expand_path filename
+ hash.merge block.call(filename) || {}
@parkr

parkr Feb 23, 2014

OOC, how does Ruby parse this? Is it:

hash.merge(block.call(filename) || {})

or

hash.merge(block.call(filename)) || {}

Which is intended? Difficult for the reader to know here.

@bkeepers

bkeepers Mar 8, 2014

Owner

Thanks, updated.

@bkeepers bkeepers added a commit that referenced this pull request Mar 8, 2014

@bkeepers bkeepers Merge pull request #96 from bkeepers/cleanup-duplication
Clean up duplication in #load, #load! and #overload
4f004aa

@bkeepers bkeepers merged commit 4f004aa into master Mar 8, 2014

@bkeepers bkeepers deleted the cleanup-duplication branch Mar 8, 2014

@sheax0r sheax0r pushed a commit to sheax0r/dotenv that referenced this pull request Sep 13, 2015

@bkeepers bkeepers Merge pull request #96 from bkeepers/cleanup-duplication
Clean up duplication in #load, #load! and #overload
38b1b59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment