Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 4 commits into from

3 participants

@kachick
Collaborator

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"
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 Owner
dbussink added a note

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 Collaborator
kachick added a note

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
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 Owner
brixen added a note

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

@kachick Collaborator
kachick added a note

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
Owner

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
Owner

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
@kachick
Collaborator

Thanks!

@kachick kachick deleted the kachick:fix/String-chomp_bang_when_replaced_DEFAULT_SEPARATOR branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 8, 2012
  1. @kachick

    Adjust spec descriptions for String#chomp, String#chomp!

    kachick authored
    "with" is not a correct word for current specs.
  2. @kachick

    Add specs for String#chomp! without argument

    kachick authored
    ported from String#chomp specs
  3. @kachick

    Fix String#chomp! without argument

    kachick authored
    String#chomp! should depend on $/ as String#chomp.
  4. @kachick
This page is out of date. Refresh to see the latest.
View
4 kernel/common/string18.rb
@@ -233,9 +233,9 @@ def chop!
# NOTE: TypeError is raised in String#replace and not in String#chomp! when
# self is frozen. This is intended behaviour.
- def chomp!(sep=undefined)
+ def chomp!(sep=$/)
# special case for performance. No seperator is by far the most common usage.
- if sep.equal?(undefined)
+ if sep == DEFAULT_RECORD_SEPARATOR
return if @num_bytes == 0
Rubinius.check_frozen
View
4 kernel/common/string19.rb
@@ -497,11 +497,11 @@ def chop!
# NOTE: TypeError is raised in String#replace and not in String#chomp! when
# self is frozen. This is intended behaviour.
- def chomp!(sep=undefined)
+ def chomp!(sep=$/)
Rubinius.check_frozen
# special case for performance. No seperator is by far the most common usage.
- if sep.equal?(undefined)
+ if sep == DEFAULT_RECORD_SEPARATOR
return if @num_bytes == 0
c = @data[@num_bytes-1]
View
92 spec/ruby/core/string/chomp_spec.rb
@@ -1,7 +1,7 @@
require File.expand_path('../../../spec_helper', __FILE__)
require File.expand_path('../fixtures/classes.rb', __FILE__)
-describe "String#chomp with separator" do
+describe "String#chomp" do
it "returns a new string with the given record separator removed" do
"hello".chomp("llo").should == "he"
"hellollo".chomp("llo").should == "hello"
@@ -44,19 +44,44 @@
"".chomp(nil).should == ""
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.chomp(sep)
+ describe "with a different $/ separator" do
+ before :each do
+ @old_rec_sep = $/
+ @expectation = lambda do |sep|
+ $/ = sep
+ [ ["", "".chomp(sep)],
+ ["x", "x".chomp(sep)],
+ ["x\n", "x\n".chomp(sep)],
+ ["x\r", "x\r".chomp(sep)],
+ ["x\r\n", "x\r\n".chomp(sep)],
+ ["x\n\r\r\n", "x\n\r\r\n".chomp(sep)],
+ ["hello", "hello".chomp(sep)]
+ ].should be_computed_by(:'chomp')
+ end
+ end
- old_rec_sep, $/ = $/, sep
+ after :each do
+ $/ = @old_rec_sep
+ end
- str.chomp.should == expected
- ensure
- $/ = old_rec_sep
- end
- end
+ it "uses separator #{"".inspect} for $/ as the separator when none is given" do
+ @expectation.call("")
+ end
+
+ it "uses separator #{"llo".inspect} for $/ as the separator when none is given" do
+ @expectation.call("llo")
+ end
+
+ it "uses separator #{"\n".inspect} for $/ as the separator when none is given" do
+ @expectation.call("\n")
+ end
+
+ it "uses separator #{"\r".inspect} for $/ as the separator when none is given" do
+ @expectation.call("\r")
+ end
+
+ it "uses separator #{nil.inspect} for $/ as the separator when none is given" do
+ @expectation.call(nil)
end
end
@@ -90,7 +115,7 @@
end
end
-describe "String#chomp! with separator" do
+describe "String#chomp!" do
it "modifies self in place and returns self" do
s = "one\n"
s.chomp!.should equal(s)
@@ -134,6 +159,47 @@
"hello".chomp!(nil).should == nil
end
+ describe "with a different $/ separator" do
+ before :each do
+ @old_rec_sep = $/
+ @expectation = lambda do |sep|
+ $/ = sep
+ [ ["", "".chomp!(sep)],
+ ["x", "x".chomp!(sep)],
+ ["x\n", "x\n".chomp!(sep)],
+ ["x\r", "x\r".chomp!(sep)],
+ ["x\r\n", "x\r\n".chomp!(sep)],
+ ["x\n\r\r\n", "x\n\r\r\n".chomp!(sep)],
+ ["hello", "hello".chomp!(sep)]
+ ].should be_computed_by(:'chomp!')
+ end
+ end
+
+ after :each do
+ $/ = @old_rec_sep
+ end
+
+ it "uses separator #{"".inspect} for $/ as the separator when none is given" do
+ @expectation.call("")
+ end
+
+ it "uses separator #{"llo".inspect} for $/ as the separator when none is given" do
+ @expectation.call("llo")
+ end
+
+ it "uses separator #{"\n".inspect} for $/ as the separator when none is given" do
+ @expectation.call("\n")
+ end
+
+ it "uses separator #{"\r".inspect} for $/ as the separator when none is given" do
+ @expectation.call("\r")
+ end
+
+ it "uses separator #{nil.inspect} for $/ as the separator when none is given" do
+ @expectation.call(nil)
+ end
+ end
+
ruby_version_is ""..."1.9" do
it "raises a TypeError when self is frozen" do
a = "string\n"
Something went wrong with that request. Please try again.