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

[cs] Misleading conditional #216

Open
sfgeorge opened this issue Mar 17, 2014 · 2 comments
Open

[cs] Misleading conditional #216

sfgeorge opened this issue Mar 17, 2014 · 2 comments

Comments

@sfgeorge
Copy link
Member

The following conditional found in lib/punchblock/component/output.rb has a misleading final elsif:

def xml_value
  if ssml?
    value.to_s
  elsif urilist?
    value.to_s
  elsif
    value
  end
end

One may think that the above elsif is equivalent to ... else value end or ... elsif value; value end, but neither is the case. As seen in this gist, it's a strange, empty elsif block with nil as a return value. The method is functionally equivalent to the following:

def xml_value
  if ssml?
    value.to_s
  elsif urilist?
    value.to_s
  elsif value
    nil
  end
end

Additionally, if we change that nil to ">FOO!" the test suite continues to pass. So while this is a private method, we should probably fix this and ensure it is tested.

What is the desired outcome of this case? value, value.to_s, or KABOOM! with a raised exception?

@benlangfeld
Copy link
Member

This is supposed to be:

def xml_value
  if ssml?
    value.to_s
  elsif urilist?
    value.to_s
  else
    value
  end
end

Tests / a fix would be lovely :)

@bklang
Copy link
Member

bklang commented Oct 23, 2014

Is there any way to trigger this bug from a public method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants