Implement syntax highlighting.

[myronmarston]

This PR adds support for syntax highlighting the printed failure snippet when the coderay gem is available. Here's how it looks:

The implementation is pretty straightforward but there is one hacky part about it: to work properly for the single-line case, where the snippet goes after Failure/Error, we have to insert an ANSI reset code (\e[0m) at the start of the lines -- otherwise there was "leakage" of color changes. What I dislike about the solution is that the syntax highlighter assumes the exception presenter is going to put the snippet at the end of the line. With that assumption, the reset code solution works OK because there isn't any additional text from the presenter later in the line that should be colored a certain way that we are screwing up. But if the snippet was interpolated in the middle of a colored string, our current implementation would lead to some weirdness.

I'm not sure what to do about it if anything, but thought it was worth noting.

[JonRowe]

This looks good to me barring a windows solution... Maybe we just make this unavailable in windows at the moment?

[myronmarston]

This looks good to me barring a windows solution... Maybe we just make this unavailable in windows at the moment?

Done. See my latest commit.

[yujinakayama]

SnippetExtractor might return non-parsable lines if the failure lines contain part of another expression:

some_expression(
  :foo
); this_is_the_failed_expression

How does CodeRay.encode work when such source lines are given?

[yujinakayama]

IMO, displaying # Install the coderay gem to get syntax highlighting for each failure is a bit annoying when the user is aware of the syntax highlight feature but not interested.

[myronmarston]

How does CodeRay.encode work when such source lines are given?

I tried it with an expression that was more than the default max of 10 lines (so that the snippet was an incomplete ruby snippet) and it syntax highlighted it OK. It seems to handle non-parsable snippets OK.

IMO, displaying # Install the coderay gem to get syntax highlighting for each failure is a bit annoying when the user is aware of the syntax highlight feature but not interested.

That's a valid point, but:

• It's not every failure. Just failures with multi-line snippets. I didn't want single-line snippets to turn into multiline just for the comment.
• This is what we've done in the HTML formatters for as long as I can remember and we've never had any complaints from users as far as I know.
• This is an easy feature to miss and not even be aware of so it's nice to let users know in some fashion.

Do you have a suggestion for an alternate way to do it?

[yujinakayama]

Do you have a suggestion for an alternate way to do it?

How about displaying the message only once using MessageNotification?

rspec-core/lib/rspec/core/notifications.rb

Lines 247 to 251 in 19761e3

	# The `MessageNotification` encapsulates generic messages that the reporter
	# sends to formatters.
	#
	# @attr message [String] the message
	MessageNotification = Struct.new(:message)

[myronmarston]

OK, I came up with a way to show the message at most once in the output. It's displayed as part of the summary. Here's what it looks like:

  11) RSpec::Core::Reporter finish includes a note about install coderay if syntax highlighting is unavailable
      Failure/Error:
        expect(formatter).to have_received(:dump_summary).with(an_object_having_attributes(
          :flly_formatted => a_string_including(install_coderay_snippet)
        ))

        #<Double "formatter"> received :dump_summary with unexpected arguments
          expected: (an object having attributes {:flly_formatted => (a string including "install the coderay gem")})
               got: (#<struct RSpec::Core::Notifications::SummaryNotification duration=3.9e-05, examples=[], failed_examples=[], pending_examples=[], load_time=11.181606, syntax_highlighting_unavailable=true>)
        Diff:
        @@ -1,2 +1,2 @@
        -["an object having attributes {:flly_formatted => (a string including \"install the coderay gem\")}"]
        +[#<struct RSpec::Core::Notifications::SummaryNotification duration=3.9e-05, examples=[], failed_examples=[], pending_examples=[], load_time=11.181606, syntax_highlighting_unavailable=true>]
      # ./spec/rspec/core/reporter_spec.rb:53:in `block (3 levels) in <module:Core>'
      # ./spec/support/sandboxing.rb:14:in `block (3 levels) in <top (required)>'
      # ./spec/support/sandboxing.rb:7:in `block (2 levels) in <top (required)>'

Syntax highlighting of failure snippets unavailable -- install the coderay gem to enable it.

Finished in 10.18 seconds (files took 1.23 seconds to load)
1887 examples, 11 failures, 1 pending

[yujinakayama]

👍

[JonRowe]

OK, I came up with a way to show the message at most once in the output. It's displayed as part of the summary.

This is really kind of ugly...

[xaviershay]

I'm not going to bring in coderay gem on all of my projects just for this feature, so I'm always going to get this message. That's kind of annoying I think.

It's a hard balance between advertising and getting out of the way!

We'd have to add a config flag for it, which seems possibly overkill.

Maybe leave it as is and I'll send you a PR if it's really getting to me ;)

[myronmarston]

It's a hard balance between advertising and getting out of the way!

We'd have to add a config flag for it, which seems possibly overkill.

Yep, I thought about the config flag, too, but yeah, that feels like overkill. We do already kinda have a config flag, anyway: --formatter: you can configure a custom formatter that avoids displaying that message. That'd be way overkill as well, though.

One other thing to bear in mind: the syntax highlighter is only attempted to be used if there's a failure, which means reporter.syntax_highlighting_unavailable is only set if there's a failure, which means that when there are no failures you won't get this message. That's intentional since I figure the user isn't missing out when everything passes. Hopefully that will make it less annoying for your case.

[myronmarston]

The build is green and I think I've addressed the feedback. Anyone want to review it so we can merge it?

[JonRowe]

I'm still not happy with the warning message, can we just remove it?

[myronmarston]

I'm still not happy with the warning message, can we just remove it?

Why has it never bothered you that we provide a similar message on every single code snippet in the HTML formatter?

rspec-core/lib/rspec/core/formatters/html_snippet_extractor.rb

Lines 10 to 14 in 78cf993

	module NullConverter
	def self.convert(code)
	%Q(#{code}\n<span class="comment"># Install the coderay gem to get syntax highlighting</span>)
	end
	end

I think it's better to have the warning message and let users know they are missing out on the syntax highlighting than to have it be a hidden feature that users have to read the release blog post or changelog to learn about.

[JonRowe]

Because that is constrained to the HTML formatter and doesn't pollute the reporter and every other formatter.

[JonRowe]

To clarify, I'm bothered by the code quality, not the warning itself, it'd be better to build a way to add this sort of message to the summary in a generic fashion (as a public api) and use that to add this warning, than do this.

[myronmarston]

To clarify, I'm bothered by the code quality, not the warning itself, it'd be better to build a way to add this sort of message to the summary in a generic fashion (as a public api) and use that to add this warning, than do this.

I'm not sure what you have in mind as an improvement. Want to push a commit with your idea to this PR?

[JonRowe]

Done. It wouldn't be too hard to add a generic "squash this message" setting to that commit either so you could disable certain types of messages.

[myronmarston]

Thanks, @JonRowe, I do like the approach in general!

[tomstuart]

Is there — or will there be — a way to disable the warning without installing coderay? Syntax highlighting is a great feature but I worry about this sort of “warning creep” in general. (I’ve never used the HTML formatter so didn’t know about it there.)

[JonRowe]

With my approach I intend to add a way to suppress these messages

On Tuesday, 10 November 2015, Tom Stuart [email protected] wrote:

Is there — or will there be — a way to disable the warning without
installing coderay? Syntax highlighting is a great feature but I worry
about this sort of “warning creep” in general. (I’ve never used the HTML
formatter so didn’t know about it there.)

—
Reply to this email directly or view it on GitHub
#2109 (comment).

[myronmarston]

I don't really want to start the process of adding a new config option for each bit of the formatter output that a user might want to silence.

We already added one in #2007 and it's got me thinking about how we can do this more generically. @JonRowe may already be thinking of this, but what about:

RSpec.configure do |c|
  c.silence_formatter_message /Run options/
  c.silence_formatter_message /coderay/
end

[yujinakayama]

I feel requiring users to add c.silence_formatter_message /coderay/ to their minimal setup is strange... Also, it's more obscure for users to know the way to silence the warning than the syntax highlight feature itself.

How about adding some comment to spec_helper.rb generated by rspec --init instead of the runtime warning?

[myronmarston]

Maybe for now we should just remove the message altogether and just do syntax highlighting if coderay is available. We can document it in release notes and the release blog post.

[JonRowe]

I've made the changes you suggested @myronmarston, my intention is to add a config option to silence these, I don't think it's needed by default but I think the reporter should mention how to silence them, e.g...

config.silence_summary_messages :coderay, :something_else

[myronmarston]

IMO, it seems odd to require the user to know what internal symbol is used to identify the message. If they can just call config.suppress_messages /some regex/ then they can suppress any notified message. Compare to an API like config.silence_summary_messages :coderay, :something_else: it only supports silencing messages notified via this new add_message_before_summary. We only have one message notified via it and we don't know how if we'll have additional messages that use that API in the future.

An API like config.suppress_messages can be used now to more generically handle existing things like config.silence_filter_announcements (from #2007).

[myronmarston]

In the interest of getting a release out, I'm leaning towards cutting the message about coderay (and any API to silence such a message) for now, with the idea that we can release without the message and always add it (and the silencing API) later. I don't want to merge a new public API just before a release if we're not in total agreement about what the API should look like.

Thoughts?

[yujinakayama]

In the interest of getting a release out, I'm leaning towards cutting the message about coderay (and any API to silence such a message) for now, with the idea that we can release without the message and always add it (and the silencing API) later.

👍

[JonRowe]

Well we could merge this as is and add the silencing api in a point release :P

[myronmarston]

Well we could merge this as is and add the silencing api in a point release :P

That assumes we're all on the same page about the add_message_before_summary(type, message) API. As I commented above, I'm not convinced the type arg is something we need or something we want to support. You might yet convince me but since it's a public API I'd prefer to pull it (and the usage of it) out of this release.

[JonRowe]

Well I've pulled the warning for now

[myronmarston]

LGTM, merge when green.

[myronmarston]

I may try to cut a release tonight. We'll see if I have time. (I have to put my kids to bed in a bit and that can take a very long time...)

[xaviershay]

pull it and that leaves us an actual feature for 4.0 :P

[JonRowe]

New in 4.0.0: Silencing all the error messages we added in 3.x? :P

[myronmarston]

New in 4.0.0: Silencing all the error messages we added in 3.x? :P

To make the upgrade to 4.0 compelling, we can add lots and lots of warnings in future 3.x releases!

[JonRowe]

To make the upgrade to 4.0 compelling, we can add lots and lots of warnings in future 3.x releases!

3.5.0 Development
* Futureproofing RSpec by adding extra warnings.