Adding trim option to ERB, closes #840 #846

Merged
merged 4 commits into from Mar 31, 2013

Conversation

Projects
None yet
3 participants
Contributor

hron84 commented Mar 26, 2013

The difference described in rspec spec:

require 'rspec/autorun'
require 'erb'

describe ERB do
  let(:strip_template) { "<% if 1 == 1 -%>\nPassed<% end -%>\n" }
  let(:normal_template) { "<% if 1 == 1 %>\nPassed<% end %>\n" }

  context 'without trim mode' do
    it 'accepts normal templates' do
      template = ERB.new(normal_template)
      expect do
        template.result(binding)
      end.to_not raise_error SyntaxError
    end

    it 'does not trim newlines' do
      template = ERB.new(normal_template)
      template.result(binding).should eql "\nPassed\n"
    end

    it 'does not accept dashes at open/close tags' do
      template = ERB.new(strip_template)
      expect do
        template.result(binding)
      end.to raise_error SyntaxError
    end
  end

  context 'with trim mode' do
    it 'accepts normal template' do
      template = ERB.new(normal_template, nil, '-')

      expect do
        template.result(binding)
      end.to_not raise_error SyntaxError
    end

    it 'accepts dashes at open/close tags' do
      template = ERB.new(strip_template, nil, '-')
      expect do
        template.result(binding)
      end.to_not raise_error SyntaxError
    end

    it 'trims newlines' do
      template = ERB.new(strip_template, nil, '-')
      template.result(binding).should eql 'Passed'
    end
  end
end
Owner

myronmarston commented Mar 26, 2013

Thanks. The specs you put above look pretty good--can you add some similar specs to the PR? That'll ensure this doesn't regress in the future as we'll have tests for it.

Contributor

hron84 commented Mar 26, 2013

@myronmarston I tought to it, but I didn't understood correctly the structure of your specs, but tonight I'll give a second try.

Contributor

hron84 commented Mar 27, 2013

@myronmarston I added a spec here - but I am not sure it is a good place for that. Please move it if not.

Contributor

hron84 commented Mar 29, 2013

@myronmarston is there a chance to merge it at next week?

@myronmarston myronmarston and 1 other commented on an outdated diff Mar 29, 2013

spec/rspec/core/configuration_options_spec.rb
@@ -378,6 +378,18 @@
expect(parse_options[:formatters]).to eq([['local']])
end
+ it "parses options file correctly if erb code has trimming options" do
+ File.open("./.rspec", "w") do |f|
+ f << "<% if 1 == 1 -%>\n"
@myronmarston

myronmarston Mar 29, 2013

Owner

if 1 == 1 seems like a convulated way to have an if true conditional...any reason not to just use if true? That more clearly states the intention, I think.

@hron84

hron84 Mar 29, 2013

Contributor

Good catch. I dunno why not used true. Fixed.

@myronmarston myronmarston and 2 others commented on an outdated diff Mar 29, 2013

spec/rspec/core/configuration_options_spec.rb
@@ -378,6 +378,18 @@
expect(parse_options[:formatters]).to eq([['local']])
end
+ it "parses options file correctly if erb code has trimming options" do
+ File.open("./.rspec", "w") do |f|
+ f << "<% if 1 == 1 -%>\n"
+ f << "--format local\n"
+ f << "<%- end %>\n"
+ end
+
+ expect do
+ expect(parse_options[:formatters]).to eq([['local']])
+ end.to_not raise_error(SyntaxError)
@myronmarston

myronmarston Mar 29, 2013

Owner

Generally, it's best not to pass an error class to expect { }.not_to raise_error, because if an error is raised, but it is not a SyntaxError, this expectation will pass. Really, it shouldn't raise any kind of error, right? Given that, and the fact that you already have an expectation in there, I think I'd remove the raise_error expectation entirely.

@hron84

hron84 Mar 29, 2013

Contributor

I narrowed down to SyntaxError because if trim option is not present, SyntaxError is raised. It is an explicite sign if actual code cannot parse the .rspec file. No other way to detect missing trim options, sadly.

However, this test code is tests trim option existence, not the quality of the actual file parsing code. If another error is raised, it should be covered by another test, not by this.

But if you still want, I can remove the concrete error class.

@myronmarston

myronmarston Mar 29, 2013

Owner

I narrowed down to SyntaxError because if trim option is not present, SyntaxError is raised. It is an explicite sign if actual code cannot parse the .rspec file. No other way to detect missing trim options, sadly.

SyntaxError is what is raised by the ruby versions and implementations you have tried this on, but are you sure it's the error raised on all implementations and all versions (including future versions...)? If any of them raise a different kind of error, this expectation will pass even if the code doesn't work. Actually, it's worse than that. Consider what happens when you change this to:

      expect do
        expect(parse_options[:formatters]).to eq([['global']])
      end.to_not raise_error(SyntaxError)

The inner expectation fails and raises a RSpec::Expectations::ExpectationNotMetError, but because this is not a SyntaxError, it passes the outer expectation...because hey, SyntaxError was not raised. Or consider what happens if we rename the parse_options helper method but forget to update it here: the inner expression will raise a NoMethodError but that's not a SyntaxError, either, so it will still pass even though the parser isn't even being run as part of this example.

I think my preference is to remove the outer expect { }.to_not raise_error expectation entirely.

@hron84

hron84 Mar 30, 2013

Contributor

Okay, but... my problem is future maintenance. As ERB is simply eval's the inner code, no special exception to detect template problems. The thrown SyntaxError's description is too non-informative, especially because it depends on the actual template code. Not so easy to debug what happened if SyntaxError raised.

What about if i split this into two example? Still unacceptable?

@samphippen

samphippen Mar 30, 2013

Member

@hron84 the real thing you're expecting though is no error whatsoever, not, specifically, no syntax error. Therefore if we have no to_not raise_error expectation, the spec will fail if any error is given. This helps us catch a lot more cases and actually makes the spec more resilient to future breakage (not less!). I'm personally in favour of completely removing the block like @myronmarston said.

Owner

myronmarston commented Mar 29, 2013

This looks pretty good. I left a couple comments.

Contributor

hron84 commented Mar 30, 2013

I removed the mentioned expectation.

myronmarston merged commit b430328 into rspec:master Mar 31, 2013

1 check failed

default The Travis build failed
Details

@myronmarston myronmarston added a commit that referenced this pull request Mar 31, 2013

@myronmarston myronmarston Update Changelog for #846.
[ci skip]
20c6aa7
Owner

myronmarston commented Mar 31, 2013

Thanks, @hron84!

@soulcutter soulcutter added a commit to soulcutter/rspec-core that referenced this pull request Apr 7, 2013

@myronmarston @soulcutter myronmarston + soulcutter Update Changelog for #846.
[ci skip]
91d4b35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment