Thank you for submitting this proposal, @lylo.
This looks good to me at first glance save for a small correction to the documentation. I suggest you specify the need for "double-quoted" value instead of just a quoted value since our parser isn't super-flexible as of now.

Thank you for submitting this proposal, @lylo. This looks good to me at first glance save for a small correction to the documentation. I suggest you specify the need for "double-quoted" value instead of just a quoted value since our parser isn't super-flexible as of now.

Good spot, thanks @ashmaroli. I've updated this in 7cfd1fa

Hey 👋 I just wondered whether there was anything I could do to push this PR forward, or is it just a matter of waiting for someone on core to find it and take a look?

I could have sworn we had this feature like 8 years ago. Was it a Pygments-only feature that Rouge only recently included?

@lylo Pinging you to inform you that this enhancement will land in the next minor release (v4.4.0).
I do have some issues the overall code in the branch but it is not your fault since you were just acting based on surrounding code. I will get back to this after Jekyll v4.3.1 ships.

Thanks @ashmaroli 🙏

@lylo Thank you for adding the new features. Needs some minor edits:

• The Scenario keyword for last scenario is intended incorrectly.
• I feel testing for "Liquid Exception: Syntax error" would be better.

Since you mentioned "succinct", I would like to request you to make the following change to cucumber scenarios you have added:

• There really is no need to have the kramdown code block with triple backticks and the configuration entry for kramdown.input. They are principally unrelated to your changes.
  (I didn't want to burden you with this earlier since you were only repeating the pattern in existing code, but since the diff has gotten bigger now, It's best not adding more noise.)

P.S. to above comment 👆:
I have edited the features/highlighting.feature on the base branch (master: 94028e7) to give a better idea of what I meant by "noise" in the earlier comment.

Why have you renamed the sample file to "test.rb" from "index.html"?

Why have you renamed the sample file to "test.rb" from "index.html"?

You can ignore that, it was reverted in a later commit

Buddy, you have made things harder for yourself by cherry-picking my commit from the master. Now you will have to undo that action before this gets merged...... The best route is to rebase to current master now.

Locally run in terminal:

git fetch upstream master
git rebase upstream/master

@ashmaroli I have a clean branch locally I can put a fresh PR up for if you prefer. I think that would be best in this circumstance, but I'll defer to you. Apologies for the additional work I've created here.

• Incorrect expectation -- Tag should expect a (double-)quoted string of numbers, not an array for hl_lines. Array of integers is a result of preliminary internal processing, not user-given.

Fixed in 03a68ea

• Insufficient -- hl_class gets flagged when quoted only if the string consists of non-numbers. What if the parser doesn't catch it? For example: highlight_line_class="2 3 4"?

I've addressed this in 289421a by relaxing the rules of the parser. So while the correct format for declaring highlight_line_class is still:

{% highlight ruby highlight_lines="1 2" highlight_line_class=one_two %}

This change will be forgiving and do the expected thing if somebody did try and pass in a double-quoted list of integers, so this:

{% highlight ruby highlight_lines="1 2" highlight_line_class="1 2" %}

Will result in a class name of "1 2"

I'm happy to be less forgiving and raise a syntax error instead if you prefer.

The main problem I'm having here is the overarching strict parsing rules and regexes. It could really do with a full overhaul. I'm just trying to work within it's constraints.

@lylo Found multiple issues with the latest implementation. I guess you couldn't give 100% focus into this due to the various moving parts and inputs from my end. I therefore took the liberty of cleaning the branch up directly instead of first pointing out all the issues.
Do run git pull origin support-line-highlighting to update your local copy.

@parkr Requesting a fresh round of review on this pull request. Thanks.

Hello @lylo, It just dawned on me that there's a possibility that users may get confused by the proposed line-highlighter parameter names.

Tl;dr

Can you think of better prefixes for the new proposed options?

Details

The tag itself is named highlight to convey the notion of syntax-highlighting.
Now if we were to ship options named highlight_lines, users may think it's a directive to apply syntax-highlighting only to the declared lines. Similar being the confusion with highlight_class.

My brain came up with using mark_* instead (akin to the HTML tag mark), but I feel its too technical and may not be intuitive for lay-users.

@ashmaroli I used highlight_lines and highlight_line_class because this is what's used in Rouge itself, but it's a fair question to ask – internal class names in a dependency doesn't necessarily make sense from a user perspective.

If we're avoiding highlight I think mark_lines and mark_class would be good alterntives. I can't think of anything better right now. emphasize is another word that would make sense.

Happy to change to one of these if you agree?

@lylo I understand your perspective and that the current implementation is simpler because of it. I guess we will have to settle for mark_* then because emphasis has a different meaning in HTML and it's a lot more keystrokes than mark.
I am sorry that your first contribution to Jekyll got a bit more complicated now.

@lylo I understand your perspective and that the current implementation is simpler because of it. I guess we will have to settle for mark_* then because emphasis has a different meaning in HTML and it's a lot more keystrokes than mark.
I am sorry that your first contribution to Jekyll got a bit more complicated now.

It's no problem, I'm enjoying the journey and learning a lot.

In 8a202c5 I've changed this to mark_lines. I've also removed the class option as per suggestion from @parkr which simplifies the code.

Thank you @lylo.
If you're interested, you may submit another PR to initialize the upstream HTMLTable formatter using keywords instead of a Ruby Hash.

@jekyllbot: merge +minor

@jekyllbot: merge +minor

Thank you @lylo. If you're interested, you may submit another PR to initialize the upstream HTMLTable formatter using keywords instead of a Ruby Hash.

Hey @ashmaroli, do you mean changing from:

Rouge::Formatters::HTMLTable.new(
  formatter,
  :css_class    => "highlight",
  :gutter_class => "gutter",
  :code_class   => "code"
)

to:

Rouge::Formatters::HTMLTable.new(
  formatter,
  css_class: "highlight",
  gutter_class: "gutter",
  code_class: "code"
 )

@lylo I did mean that, but I have already made the changes to master.