Add snippet type to support different type of ruby snippets. #390

rdvdijk · 2013-03-07T08:53:36Z

Take a look at these issues: cucumber/common#328 and cucumber/common#331.

The change

Ruby snippets have now changed from this form:

When /^there is a missing step$/ do
  pending # express the regexp above with the code you wish you had
end

to:

When(/^there is a missing step$/) do
  pending # express the regexp above with the code you wish you had
end

The choice

This pull request gives the command line option to change Ruby snippet formats.

Using --snippet-type regexp is the default, and will create snippets with parentheses.
Using --snippet-type legacy will create snippets the old way, without parentheses.
Using --snippet-type percent will create snippets the with percent-style regular expressions:

When %r{^there is a missing step$} do
  pending # express the regexp above with the code you wish you had
end

The disclaimer

I'm not too happy about adding a fourth argument to the Cucumber::RbSupport::RbLanguage#snippet_text function and the same function in all other supported languages. I couldn't find another way to get to this option, I hope this is acceptable.

…hout parentheses or %r style).

mattwynne · 2013-03-25T22:58:40Z

Thanks for your contribution @rdvdijk, I've had a chance to review it and I have a few comments.

The main one is that I'd like to see this patch use polymorphism rather than symbol keys for the different snippet types. I think that would also allow us to push the list of available snippet types down into the language. After 2.0, we'll only support Ruby and Wire languages anyway.

I'll add some comments to the code to point out what I mean.

mattwynne · 2013-03-25T23:08:40Z

lib/cucumber/rb_support/rb_language.rb

@@ -184,6 +191,17 @@ def check_nil(o, proc)
          o
        end
      end
+
+      def typed_snippet_pattern(pattern, type)


Have this method do a hash lookup to the class that represents the snippet type:

{ regexp: Snippet::Regexp, legacy: Snippet::Legacy, percent: Snippet::Percent, }.fetch(type).new(pattern)

Now you could move the code from lines 104-111 onto a base clases for a Snippet.

mattwynne · 2013-03-25T23:09:29Z

I hope those comments make sense - sorry for the delay. Just shout if you have any questions.

rdvdijk · 2013-03-26T06:01:24Z

That makes a lot of sense. I had considered doing this, but was worried I would have to change too much code to make it work. I especially dislike passing around so many variables down to where the actual snippets are rendered.

I'll look into this, thanks for the review.

mattwynne · 2013-03-26T09:42:59Z

Red, green, refactor. You've done the hard work and got this change to green, now the refactoring is the fun part!

I think pulling the snippet rendering out of the RbLanguage class will be a great improvement.

rdvdijk · 2013-03-27T08:48:14Z

I've not only pulled all the snippet rendering into a base snippet class. I've moved the tests to a separate spec as well. Any more comments? Let me know if you'd like me to refactor some more 😄

rdvdijk · 2013-03-27T08:50:02Z

Ah, I broke 1.8 compatibility. Let me fix that.

mattwynne · 2013-03-27T09:06:24Z

lib/cucumber/rb_support/rb_language.rb

-        if(multiline_arg_class == Ast::Table)
-          multiline_class_comment = "# #{multiline_arg_class.default_arg_name} is a #{multiline_arg_class.to_s}\n  "
-        end
+        snippet.code_keyword = code_keyword


Could you either pass these three parameters (code_keyword, parttern, multiline_argument_class) into the snippet constructor, or pass them into #render? I find code is much easier to reason about when objects are as immutable as possible.

mattwynne · 2013-03-27T09:07:24Z

Much better! Thanks for reworking this.

rdvdijk · 2013-03-27T09:08:45Z

I fiddled with a builder pattern before settling on the current version. But I guess three arguments into a constructor isn't that bad,

I'll change it.

rdvdijk · 2013-03-27T09:37:54Z

Here you go! More comments?

mattwynne · 2013-03-27T10:46:33Z

lib/cucumber/rb_support/snippet.rb

+
+      class BaseSnippet
+
+        attr_reader :code_keyword, :pattern, :multiline_argument_class


These can be made private, right?

rdvdijk · 2013-03-27T12:37:52Z

There, more clean up. I think we're getting there.

mattwynne · 2013-03-27T13:07:29Z

Looking good! Now how about calling replace_and_count_capturing_groups! from the constructor, passing it the original pattern? Then we won't have that ugly mutation of the @pattern instance variable. And you could inline render_snippet into render (or even just call it to_s)

rdvdijk · 2013-03-27T13:15:11Z

Your wish is my command! 😉

mattwynne · 2013-03-27T14:23:40Z

spec/cucumber/rb_support/rb_language_spec.rb

-          })
+        it "creates a legacy Snippet class" do
+          Snippet::Legacy.stub(:new => stub.as_null_object)
+          Snippet::Legacy.should_receive(:new)


I think you could change these two lines to:

Snippet::Legacy.should_receive(:new).and_return(snipped)

mattwynne · 2013-03-27T14:26:01Z

Really great contribution @rdvdijk - thanks for having the patience to iterate on it with me!

Add snippet type to support different type of ruby snippets.

rdvdijk · 2013-03-27T14:29:23Z

Thanks for all the help, and for merging, @mattwynne! 🎉

mattwynne · 2013-03-27T14:30:29Z

lib/cucumber/cli/options.rb

+                  "Use different snippet type (Default: regexp). Available types:",
+                  "regexp  : Snippets with parentheses,    e.g. \"When(/^missing step$/) do\"",
+                  "legacy  : Snippets without parentheses, e.g. \"When /^missing step$/ do\"",
+                  "percent : Snippets with percent regexp, e.g. \"When %r{^missing step$} do\"") do |v|


Once refactoring enhancement I think you could consider doing in a separate PR is to move these descriptions down to the Snippet classes, and expose the list of snippet types (the values from the hash in the factory method) as a method on RbLanguage. Then this option parser would not have to know anything about the different types of snippets, and we could easily add more without having the change the option parser.

Do you see what I mean?

That makes a lot of sense! I'll work on it somewhere in the coming days.

lock · 2018-10-25T16:29:50Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Add snippet type to support different type of ruby snippets (with/wit…

88580ae

…hout parentheses or %r style).

mattwynne reviewed Mar 25, 2013
View reviewed changes

Pull snippet rendering out of RbLanguage.

1a668f1

mattwynne reviewed Mar 27, 2013
View reviewed changes

Roel van Dijk added 2 commits March 27, 2013 10:09

Fix 1.8 compatibility.

768f7fb

Make snippet-class stateless.

6974c65

mattwynne reviewed Mar 27, 2013
View reviewed changes

Make attr_reader private, clean up some more code.

92b3e37

Some more code cleanup 👍

3373c65

mattwynne reviewed Mar 27, 2013
View reviewed changes

mattwynne added a commit that referenced this pull request Mar 27, 2013

Merge pull request #390 from rdvdijk/snippet_type

a667b4b

Add snippet type to support different type of ruby snippets.

mattwynne merged commit a667b4b into cucumber:master Mar 27, 2013

mattwynne reviewed Mar 27, 2013
View reviewed changes

lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snippet type to support different type of ruby snippets. #390

Add snippet type to support different type of ruby snippets. #390

rdvdijk commented Mar 7, 2013

mattwynne commented Mar 25, 2013

mattwynne Mar 25, 2013

mattwynne commented Mar 25, 2013

rdvdijk commented Mar 26, 2013

mattwynne commented Mar 26, 2013

rdvdijk commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

mattwynne commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

mattwynne commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

rdvdijk Mar 27, 2013

lock bot commented Oct 25, 2018


		class BaseSnippet

		attr_reader :code_keyword, :pattern, :multiline_argument_class

Add snippet type to support different type of ruby snippets. #390

Add snippet type to support different type of ruby snippets. #390

Conversation

rdvdijk commented Mar 7, 2013

The change

The choice

The disclaimer

mattwynne commented Mar 25, 2013

mattwynne Mar 25, 2013

Choose a reason for hiding this comment

mattwynne commented Mar 25, 2013

rdvdijk commented Mar 26, 2013

mattwynne commented Mar 26, 2013

rdvdijk commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

Choose a reason for hiding this comment

mattwynne commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

Choose a reason for hiding this comment

rdvdijk commented Mar 27, 2013

mattwynne commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

Choose a reason for hiding this comment

mattwynne commented Mar 27, 2013

rdvdijk commented Mar 27, 2013

mattwynne Mar 27, 2013

Choose a reason for hiding this comment

rdvdijk Mar 27, 2013

Choose a reason for hiding this comment

lock bot commented Oct 25, 2018