Project

General

Profile

Actions

Bug #16173

closed

ENV.delete returns nil when name does not exist and block given

Added by burdettelamar@yahoo.com (Burdette Lamar) over 4 years ago. Updated almost 4 years ago.

Status:
Closed
Assignee:
-
Target version:
-
ruby -v:
ruby 2.6.4p104 (2019-08-28 revision 67798) [x64-mingw32]
[ruby-core:95008]
Tags:

Description

Attached diff.txt:

  • ENV.delete for nonexistent name and block given:
    • Test enhanced to verify return value is nil.
    • Documentation corrected to say that return value is nil, not value.

Files

diff.txt (2.22 KB) diff.txt burdettelamar@yahoo.com (Burdette Lamar), 09/21/2019 05:08 PM

Updated by alanwu (Alan Wu) over 4 years ago

This gives the impression that the return value is always nil, which isn't true when the environment variable exists.
Maybe it's worth clarifying that.

Updated by nobu (Nobuyoshi Nakada) over 4 years ago

Comparing with Hash#delete, it looks that the document is correct and the code is wrong.

diff --git i/hash.c w/hash.c
index 8b84a14484..7880178dc8 100644
--- i/hash.c
+++ w/hash.c
@@ -4779,7 +4779,7 @@ env_delete_m(VALUE obj, VALUE name)
     VALUE val;
 
     val = env_delete(name);
-    if (NIL_P(val) && rb_block_given_p()) rb_yield(name);
+    if (NIL_P(val) && rb_block_given_p()) val = rb_yield(name);
     return val;
 }
 
diff --git i/test/ruby/test_env.rb w/test/ruby/test_env.rb
index b01c3b12ee..1a7656ea7d 100644
--- i/test/ruby/test_env.rb
+++ w/test/ruby/test_env.rb
@@ -107,6 +107,7 @@
     assert_invalid_env {|v| ENV.delete(v)}
     assert_nil(ENV.delete("TEST"))
     assert_nothing_raised { ENV.delete(PATH_ENV) }
+    assert_equal("NO TEST", ENV.delete("TEST") {|name| "NO "+name})
   end
 
   def test_getenv

Updated by burdettelamar@yahoo.com (Burdette Lamar) over 4 years ago

Thanks, @nobu (Nobuyoshi Nakada), but I'm not going to propose a change to the functionality.

Updated by burdettelamar@yahoo.com (Burdette Lamar) over 4 years ago

Thanks, @alanwu (Alan Wu). I'm refreshing diff.txt with more fulsome documentation, along with enhanced testing.

Actions #5

Updated by burdettelamar@yahoo.com (Burdette Lamar) over 4 years ago

  • File deleted (diff.txt)

Updated by Eregon (Benoit Daloze) over 4 years ago

IMHO it's better to fix behavior to be consistent with Hash#delete.
And the compatibility risk seems non existent here.

So I think we should change behavior, and not change documentation which would be inconsistent with Hash#delete.
+1 for @nobu's patch.

Updated by burdettelamar@yahoo.com (Burdette Lamar) over 4 years ago

I, being a member only of The Outer Party, can't merge anything here.

I'm agnostic on whether the change should be to the documentation or to the code. My part was just to note the discrepancy.

Can someone in The Inner Party take action?

Thanks,
Burdette

Updated by jeremyevans0 (Jeremy Evans) over 4 years ago

I also agree that nobu's patch should be merged.

Updated by nobu (Nobuyoshi Nakada) over 4 years ago

(Burdette Lamar) wrote:

Thanks, @alanwu (Alan Wu). I'm refreshing diff.txt with more fulsome documentation, along with enhanced testing.

Thank you for the patch.

+ * Deletes the environment variable for +name+ if it exists (ignoring the block, if given); returns +nil+:
+ *   ENV.delete('LINES') # => '300'
+ *   ENV.delete('COLUMNS') { |name| fail 'boo' } # => '120'

It should return the old value, not nil.

+ * Calls the block and returns +nil+ if the environment variable does not exist and block given:
+ *   ENV.delete('NOSUCH') { |name| } # => nil

Non-empty block and non-nil result feels better to me.

Actions #11

Updated by nobu (Nobuyoshi Nakada) almost 4 years ago

  • Status changed from Open to Closed

Applied in changeset git|04fddf35734f04fd16824a847cad499465663a5f.


ENV.delete should return the result of block on non-existing key

Fixes [Bug #16173]

Co-Authored-By: Burdette Lamar
Co-Authored-By: Jeremy Evans

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0