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

Allow Spec's .pending and .it to accept constant #7646

Conversation

straight-shoota
Copy link
Member

This enables to use pending and it with a constant as description argument:

pending Foo do
end
it Foo do
end

This already works for describe and context, but for both pending and it raised an error because of mismatching types.

@konovod
Copy link
Contributor

konovod commented Apr 7, 2019

Isn't spec name supposed to be a clear description of what test do?
Maybe I'm missing some cases, but
Char::Reader raises IndexError when smth - nice name for a test
Char::Reader IndexError - not nice.

@straight-shoota
Copy link
Member Author

@konovod Yeah, I'm not sure if it makes much sense for it to use a constant as description. But pending should definitely support it because you can simply convert a describe or context to pending. Given that all the other methods support a constant, it's better to have it on it as well.

@straight-shoota
Copy link
Member Author

Maybe a useful example for it:

describe "#read" do
  it Int { }
  it String { }
end

@Sija
Copy link
Contributor

Sija commented Apr 7, 2019

I wouldn't go for it but pending makes sense.

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't hurt to just allow

@straight-shoota straight-shoota merged commit 02fde16 into crystal-lang:master Apr 7, 2019
@straight-shoota straight-shoota added this to the 0.28.0 milestone Apr 7, 2019
@straight-shoota straight-shoota deleted the fix/spec-pending-description branch April 7, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants