Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cucumber is generating Ruby step definition snippets that cause Ruby warnings. #328

Closed
martco opened this issue Oct 12, 2012 · 11 comments
Closed

Comments

@martco
Copy link
Contributor

martco commented Oct 12, 2012

Steps to Recreate

  1. If I save a file called addition.feature that has the following line inside of it:

    Given I have entered 50 into the calculator
    
  2. And at the command line I run:

    cucumber features/addition.feature
    
  3. Cucumber will tell me that I haven’t implemented the step defintion, and it provides me with a snippet to move me along.

    You can implement step definitions for undefined steps with these snippets:
    
    Given /^I have entered (.*) into the calculator$/ do |n|
      pending # express the regexp above with the code you wish you had         
    end
    

    This is really useful

  4. However, when I check the ruby syntax of features/addition.feature with warnings on:

    ruby -cw features/addition.feature
    

    I get the following warning:

    features/addition.feature:1: warning: ambiguous first argument; put parentheses or even spaces
    
  5. And whenever I save a file that has many Cucumber-generated snippets inside of it, I see this in my vim Quickfix window (which is checking syntax with warnings on):

    This is alarming

Who Cares

One response to this might be, “Who cares? Not my problem, if you don’t want to see the warnings, turn off verbose mode. Verbose mode is broken.”

I might agree with that while writing closed-source stuff, but I disagree when considering open source software. As @mislav writes (emphasis mine):

If you’re a developer of open source code you should regularly check that your code doesn’t generate ruby warnings. If you accomplish this, you allow users of your code to use it in their projects with the verbose mode on and not get warned by potential threats in your codebase.

Addressing the Issue

The ruby warning is happening because the first / of the Cucumber-generated snippet could signify either a division or a regular expression. I’ve tested a fix that wraps the regular expression in parentheses and the warning is gone. This could happen in-line or perhaps could be activated by a setting. I’ve got a pull request lined-up, but wanted to check here first for initial feedback or thoughts.

Thanks. I think the snippet-generation is really useful, and I would love to continue using it with, at the very least, an option to generate ruby-safe snippets.

@mattwynne
Copy link
Member

So two options here:

  1. put brackets around the regexp
Given(/foo/) do
  # whatever
end
  1. use another way of marking up the regexp
Given %r{foo} do
  # whatever
end
  1. who cares

I care. I think I like option 1 best. What do other people think?

@mattwynne
Copy link
Member

Oh wait that's three options. :)

@martco
Copy link
Contributor Author

martco commented Oct 12, 2012

I like 1 because:

  1. I've been using it locally and it works fine
  2. It's what the ruby warning suggests
  3. It has the least impact on the code

@martco
Copy link
Contributor Author

martco commented Oct 12, 2012

but actually, now that I consider it further, i think the %r { ... } literal is a better choice than regex with parens:

%r literal Regex with parens
%r{Lorem ipsum} (/^Lorem ipsum$/)

It seems more legible and should still address the issue.

@mattwynne
Copy link
Member

Are those two examples the same? Are the anchors not required with the %r{} form?

@martco
Copy link
Contributor Author

martco commented Oct 15, 2012

They're not the same. I tested earlier and was going to mention that. I tried using %r{} and was getting ambiguous mismatch errors. I think the first option is best.

@martco
Copy link
Contributor Author

martco commented Oct 15, 2012

To better answer your question, I didn't use the anchors when testing. So right now that would lead me to believe that they are necessary.

@martco
Copy link
Contributor Author

martco commented Oct 19, 2012

@mattwynne i opened a pull request here

@mattwynne
Copy link
Member

Thanks!

@os97673
Copy link
Member

os97673 commented Feb 18, 2013

Fixed by PR #331

@lock
Copy link

lock bot commented Oct 25, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants