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 .fetch to ENV #1533

Merged
merged 1 commit into from Sep 21, 2015
Merged

Add .fetch to ENV #1533

merged 1 commit into from Sep 21, 2015

Conversation

@tristil
Copy link
Contributor

@tristil tristil commented Sep 20, 2015

Adds a fetch method to ENV with same interface as Hash#fetch.

@tristil tristil force-pushed the tristil:env_fetch branch 3 times, most recently from cabf31d to 022d6d6 Sep 20, 2015
@@ -79,6 +79,10 @@ class Hash(K, V)
!!find_entry(key)
end

# Fetch an environment variable corresponding to a key. Throws an exception

This comment has been minimized.

@waterlink

waterlink Sep 20, 2015
Contributor

This comment looks like just copied from ENV.fetch. It talks about environment variables, instead of normal hash.

This comment has been minimized.

@tristil

tristil Sep 20, 2015
Author Contributor

oh good catch

#
def self.fetch(key)
fetch(key) do
if block = @@block

This comment has been minimized.

@jhass

jhass Sep 20, 2015
Member

Where would @@block ever come from?

This comment has been minimized.

@tristil

tristil Sep 20, 2015
Author Contributor

I dunno :) I took this from hash.cr, where it's @block. The compiler made me change this to @@block. I thought that @block was just magic to get access to the block and that @@block was the equivalent for classes, but thinking about it now it's probably just something that was saved off to an instance variable elsewhere. The tests pass but I guess that's because it's going through line 75. I'll look at hash.cr to understand better where that variable came from.

This comment has been minimized.

@jhass

jhass Sep 20, 2015
Member

Yes, @block is just an instance variable. It should be the hash's default proc if existent. No such thing for ENV.

def self.fetch(key)
return self[key] if has_key?(key)
yield(key).to_s
end

This comment has been minimized.

@jhass

jhass Sep 20, 2015
Member

I think docs for these two would be much more interesting ;)

This comment has been minimized.

@jhass

jhass Sep 20, 2015
Member

Also reimplementing [], and []? in terms of this one should DRY up the class a little.

This comment has been minimized.

@tristil

tristil Sep 20, 2015
Author Contributor

This is just copied from hash.cr with small modifications. I wasn't sure if you usually document all the overloads but I guess why not?

This comment has been minimized.

@tristil

tristil Sep 20, 2015
Author Contributor

I'll work on reimplementing those.

raise KeyError.new "Missing ENV key: #{key.inspect}"
end
end
end

This comment has been minimized.

@jhass

jhass Sep 20, 2015
Member

I've actually been thinking about dropping this overload from Hash since it's redundant with []

This comment has been minimized.

@ysbaddaden

ysbaddaden Sep 20, 2015
Contributor

I thought about that, to reduce aliases. But fetch could be the method and [] a convenience accesor.

No strong opinion, except that fetch(key, default) is nice. But then, no fetch(key) may be weird.

@tristil tristil force-pushed the tristil:env_fetch branch from 022d6d6 to 250abb2 Sep 20, 2015
@tristil
Copy link
Contributor Author

@tristil tristil commented Sep 20, 2015

Responded to all comments. Ready for re-review.

def self.fetch(key)
value = self[key]?
return value if value
yield(key).to_s

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

If the block returns nil, Nil#to_s will turn it into "". I don't think that's expected by the caller.

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

I think I also would have this method contain the main logic and []? delegate to it too then.

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

To add some safety, maybe type restrict the block: def fetch(key : String, &block : String -> String?). You should still be able to use yield key afterwards.

This comment has been minimized.

@ysbaddaden

ysbaddaden Sep 21, 2015
Contributor

ENV.fetch("KEY", 1).to_i won't be valid then, but maybe it's a bad pattern.

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

However ENV.fetch("foo", "1").to_i would still be valid, and likewise ENV["foo"]?.try(&.to_i) || 1

This comment has been minimized.

@tristil

tristil Sep 21, 2015
Author Contributor

Ah interesting. I like the type restriction option a lot. Totally new way to think for me.


# Retrieve a value corresponding to a key. Return the value of the block if
# the key does not exist.
#

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

Mmmh, I haven't seen that doc style with the empty trailing comment seen anywhere else in the codebase ;)

@tristil tristil force-pushed the tristil:env_fetch branch 2 times, most recently from 9860f7a to 7affb72 Sep 21, 2015
ENV.fetch("2") { |k| k + "block" }.should eq("2block")
end

it "coerces the value of the block to a string" do

This comment has been minimized.

@tristil

tristil Sep 21, 2015
Author Contributor

@jhass I was planning to drop this test rather than rewriting it because I'm guessing you don't test explicit type restrictions, but when I remove 66 and 67 I get a segmentation fault when running the spec.

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

Maybe we manged to introduce some endless recursions somewhere?

@tristil
Copy link
Contributor Author

@tristil tristil commented Sep 21, 2015

Made changes from last round of suggestions. I'll be back on tonight to work on this.

ENV LIBRARY_PATH /opt/crystal/embedded/lib
ENV PATH /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

WORKDIR /opt/crystal/host

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

Uhm, how did this end up here?

This comment has been minimized.

@tristil

tristil Sep 21, 2015
Author Contributor

Sorry, I was using it in a previous commit. I'll move it out.

@jhass
Copy link
Member

@jhass jhass commented Sep 21, 2015

Yup, it's a stack overflow, the following patch makes the tests pass

diff --git a/spec/std/env_spec.cr b/spec/std/env_spec.cr
index 63a7055..0344114 100644
--- a/spec/std/env_spec.cr
+++ b/spec/std/env_spec.cr
@@ -61,12 +61,6 @@ describe "ENV" do
       ENV.fetch("2") { |k| k + "block" }.should eq("2block")
     end

-    it "coerces the value of the block to a string" do
-      ENV["1"] = "2"
-      ENV.fetch("1") { |k| k.to_i + 2 }.should eq("2")
-      ENV.fetch("2") { |k| k.to_i + 2 }.should eq("4")
-    end
-
     it "fetches and raises" do
       ENV["1"] = "2"
       expect_raises KeyError, "Missing ENV key: \"2\"" do
diff --git a/src/env.cr b/src/env.cr
index 0c62457..bc52116 100644
--- a/src/env.cr
+++ b/src/env.cr
@@ -63,8 +63,8 @@ module ENV
   # Retrieve a value corresponding to a key. Return the value of the block if
   # the key does not exist.
   def self.fetch(key: String, &block : String -> String?|NoReturn)
-    value = self[key]?
-    return value if value
+    value = LibC.getenv key
+    return String.new(value) if value
     yield(key)
   end
@tristil tristil force-pushed the tristil:env_fetch branch 2 times, most recently from 30cf9c7 to 4252ffa Sep 21, 2015
+ value = LibC.getenv key
+ return String.new(value) if value
yield(key)
endÛ201~

This comment has been minimized.

@jhass

jhass Sep 21, 2015
Member

Accidental commit I guess ;)

@tristil tristil force-pushed the tristil:env_fetch branch from 4252ffa to 52abc4b Sep 21, 2015
Adds a fetch method to ENV with same interface as Hash#fetch.
@tristil tristil force-pushed the tristil:env_fetch branch from 52abc4b to ed4d498 Sep 21, 2015
@jhass
Copy link
Member

@jhass jhass commented Sep 21, 2015

And that rebase got wrong I'm afraid. Try

git remote add upstream https://github.com/manastech/crystal.git
git fetch upstream
git checkout env_fetch
git rebase -i upstream/master # and throw everything except your commit out

Edit: oh too slow, nvm :D

@tristil
Copy link
Contributor Author

@tristil tristil commented Sep 21, 2015

Sorry for the chaos. Combination of multi-tasking with work, over-eager git commit -a, and wanting to check that tests were passing in Docker by rebasing onto my other branch. I'll slow it down next time.

@jhass
Copy link
Member

@jhass jhass commented Sep 21, 2015

No worries I can hit the cancel button on Travis a couple of times just fine :P

jhass added a commit that referenced this pull request Sep 21, 2015
@jhass jhass merged commit 1ba260a into crystal-lang:master Sep 21, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhass
Copy link
Member

@jhass jhass commented Sep 21, 2015

Thanks!

@tristil
Copy link
Contributor Author

@tristil tristil commented Sep 21, 2015

Thanks @jhass @waterlink for working me on this. Learned a lot of new concepts along the way. Next time will be smoother hopefully :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants