Call :expression on node instead of :keyword loc

[rrosenblum]

This is a continuation/replacement of #2414.

Two things have to be done:

• there should be some indication in most formatters that the highlighted code spans beyond the line we've shown
• offenses should keep track of both the start and the end of the highlighted code region, so tools like CodeClimate could query this information.

I have modified the formatters to include ellipses (...) at the end of the highlighted code if the highlighted code spans more than one line. This could open up the html formatter to have an expand button that could show the entire highlighted offense.

I will work on adding highlighted_area to the offense.

[jonas054]

I wonder why we're highlighting more than def a here. A bug in the cop?

[bbatsov]

Guess we should check this out.

[jonas054]

It's here that we highlight the whole node, in this case a def node. I wouldn't call it a bug. It's the easiest solution.

[bbatsov]

I'm not sure ellipses are the way to go, as they don't see to imply strongly enough the omitted code is on a subsequent line, but I'm not sure what we can use instead.

[rrosenblum]

I agree that ellipses do not imply that the code is on a different line. I am not sure that it necessarily matters though. Ellipses imply that there is more code included in the offense that is not shown. One alternative that we could do is add a line count to the end of the highlighted offense.

# source code
def some_method
  does_something
  does_something_else
end

An offense highlight could be

def some_method... + 3 lines
# or
def some_method + 3 lines

This may take me a little while to finish up. I have been in the process of moving and have not had too much free time recently.

[bbatsov]

Well, everything beats nothing. There's one problem with ... - what if the line ends like this something.? At the very least we should colour them.

[rrosenblum]

I could add a space between the end of the line and the ellipses. It could be something. .... I like the color idea. Do you have a color preference.

[bbatsov]

I could add a space between the end of the line and the ellipses. It could be something. ....

Guess so. Or . (...). Some light colour would do.

[bbatsov]

P.S. I like the html formatter idea as well.

[rrosenblum]

I have a basic implementation for highlighted_coded in offense. I still need to change HTMLFormatter to use the new method. I have also been playing around with the idea of changing highlighted_code to a range or also including a highlighted_range method.

[bbatsov]

OK, let me know when this is ready to be reviewed.

[rrosenblum]

Will do. Sorry this has taken so long to get running. I should have this done shortly. Everything is functioning as expected. I still have a failing test. Also, as mentioned before, I think I am going to add a highlight_range method to offense. The main question with that is will it replace highlighted_code or go in as an addition.

[rrosenblum]

This is finally functionally complete and ready for review.

[jonas054]

The reason why coloring is enabled in this case can be found in spec_helper.rb. There we have the setting Rainbow.enabled = false, but it comes after require 'rubocop', which means that ELLIPSES = Rainbow('...').yellow.freeze has already been executed when we disable coloring.

[jonas054]

I made a comment about disabling Rainbow. Fix that if you want.
Otherwise it looks good!

[rrosenblum]

@jonas054 thanks for the info on why the test was printing in color. In order to fix it, I had to require Rainbow in the spec_helper.rb and disable it before we require RuboCop.

[bbatsov]

The changes look good. Just add some changelog entry.

[rrosenblum]

I have added changelog entries. I also changed the name of the method from highlighted_range to highlighted_area. I thought this naming made more sense. If you prefer highlighted_range, I have no problem in changing it back.

[rrosenblum]

@BlakeWilliams, FYI, this has been merged.