Skip to content

String#chomp! should depend on $/ when argument not given. #2085

Closed
wants to merge 4 commits into from

3 participants

@kachick
Rubinius member
kachick commented Dec 7, 2012

MRI

  • ruby 2.0.0dev (2012-12-07) [x86_64-linux]
  • ruby 1.9.3p332 (2012-11-15) [x86_64-linux]
  • ruby 1.8.7 (2012-10-12 patchlevel 371) [x86_64-linux]
string = 'abc'
$/ = 'c'
string.chomp! #=> "ab"
string #=> "ab"

Rubinius

  • rubinius 2.0.0rc1 (1.9.3 6056ba1 2012-11-02 JI) [x86_64-unknown-linux-gnu]
  • rubinius 2.0.0rc1 (1.8.7 6056ba1 2012-11-02 JI) [x86_64-unknown-linux-gnu]
string = 'abc'
$/ = 'c'
string.chomp! #=> nil
string #=> "abc"
@dbussink dbussink and 1 other commented on an outdated diff Dec 7, 2012
spec/ruby/core/string/chomp_spec.rb
@@ -134,6 +134,22 @@
"hello".chomp!(nil).should == nil
end
+ it "uses $/ as the separator when none is given" do
+ ["", "x", "x\n", "x\r", "x\r\n", "x\n\r\r\n", "hello"].each do |str|
+ ["", "llo", "\n", "\r", nil].each do |sep|
+ begin
+ expected = str.dup.chomp!(sep)
+
+ old_rec_sep, $/ = $/, sep
@dbussink
Rubinius member
dbussink added a note Dec 7, 2012

Please use a before and after block for setting these up. It probably means that the spec should be encapsulated in a describe block, so something like this:

describe "with a different $/ separator" do
  before do
    @old_rec_sep = $/
  end

  after do
    $/ = @old_rec_sep
  end

  it "uses $/ as the separator when none is given" do
    ...
  end
end
@kachick
Rubinius member
kachick added a note Dec 8, 2012

Thanks for the pointing out.
I have used the before-after blocks in new commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brixen brixen and 1 other commented on an outdated diff Dec 7, 2012
spec/ruby/core/string/chomp_spec.rb
@@ -134,6 +134,22 @@
"hello".chomp!(nil).should == nil
end
+ it "uses $/ as the separator when none is given" do
+ ["", "x", "x\n", "x\r", "x\r\n", "x\n\r\r\n", "hello"].each do |str|
+ ["", "llo", "\n", "\r", nil].each do |sep|
@brixen
Rubinius member
brixen added a note Dec 7, 2012

Please see the be_computed_by matcher and write explicit examples, not loops.

@kachick
Rubinius member
kachick added a note Dec 8, 2012

Thanks for the pointing out.
I have tried to use the matcher be_computed_by in new commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbussink
Rubinius member

I'm wondering, why would we need to test more than one separator? I don't really see any additional value of testing multiple values over just one different value for $/. Am I missing something?

@brixen
Rubinius member
brixen commented Dec 13, 2012

I'm going to rewrite these specs. They are too complicated to explain and the code samples will explain it more effectively.

@brixen brixen closed this in d2e3fd2 Feb 13, 2013
@kachick
Rubinius member
kachick commented Feb 13, 2013

Thanks!

@kachick kachick deleted the kachick:fix/String-chomp_bang_when_replaced_DEFAULT_SEPARATOR branch Mar 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.