Skip to content

Use more standard rspec in cramped include spec #792

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

Merged
merged 3 commits into from
Apr 9, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 4, 2019

Replaces the more junit like tests in the cramped include spec with
descriptive contexts which is more idiomatic rspec.

Replaces the more junit like tests in the cramped include spec with
descriptive contexts which is more idiomatic rspec.
@nik9000 nik9000 requested a review from estolfo April 4, 2019 23:16
@@ -8,7 +8,6 @@
before(:each) do
Asciidoctor::Extensions.register do
preprocessor CrampedInclude
preprocessor ElasticCompatPreprocessor
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't needed.

it 'renders both callout lists' do
expect(converted).to include('<callout arearefs="CO1-1">')
.and(include('<callout arearefs="CO2-1">'))
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you didn't write it like the following?

expect(converted).to include('<callout arearefs="CO1-1">')
expect(converted).to include('<callout arearefs="CO2-1">')

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose the error message is a little more descriptive the way I have it, but not much. Either way is fine by me. If you prefer your way I'll switch it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's pretty minor, so whatever makes most sense to you.

@nik9000 nik9000 merged commit 5a414ed into elastic:master Apr 9, 2019
@nik9000
Copy link
Member Author

nik9000 commented Apr 9, 2019

Thanks for reviewing @estolfo!

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

Successfully merging this pull request may close these issues.

2 participants