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

Add `highlight` `start_line` option to specify starting line number #7084

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@gasi
Copy link

gasi commented Jun 19, 2018

When including partial code snippets from a larger source file, it can be useful to specify at which line the code starts. This adds option start_line=<n> to highlight Liquid tag.

I was on the fence between choosing Pygment linenostart and Rouge start_line option and went with the latter for readability, but I am open to changing it if project lead(s) feel strongly.

Example

{% highlight haskell linenos start_line=9 %}
{% include includelines filename="examples/type-driven-development-1/purescript/src/WorldCup1.purs.error" start=9 %}
{% endhighlight %}

type-driven_development__null_ _rtfm___daniel_gasienica

Add `start_line` option to specify starting line number
When including partial code snippets from a larger source file, it can be
useful to specify at which line the code starts.

@gasi gasi force-pushed the gasi:rouge-start_line branch from 85ebd73 to 7a46ee9 Jun 19, 2018

@gasi

This comment has been minimized.

Copy link

gasi commented Jun 20, 2018

@DirtyF @pathawks Would you mind taking a look? Happy to make any changes needed to land this. Thanks!

@DirtyF DirtyF requested a review from jekyll/core Jun 20, 2018

@oe

oe approved these changes Jun 20, 2018

Copy link
Member

oe left a comment

This looks pretty good, thanks for proposing this!

Show resolved Hide resolved lib/jekyll/tags/highlight.rb

@oe oe added this to the 4.0 milestone Jun 20, 2018

@oe oe added the feature label Jun 20, 2018

@@ -198,6 +214,14 @@ def highlight_block_with_opts(options_string)
@result
)
end

should "render markdown with pygments with line numbers and custom start line" do

This comment has been minimized.

@DirtyF

DirtyF Jun 20, 2018

Member

Note: Support for Pygment will be dropped in 4.0

This comment has been minimized.

@gasi

gasi Jun 20, 2018

Thanks, that’s good to know. That might factor into API naming as linenos comes from Pygment (see below).

@DirtyF
Copy link
Member

DirtyF left a comment

Quick test without an include: it modifies the line numbers but always start at the beginning of the code snippet.

1. def hello()
2.   puts "hello"
3. end

with start_line=42:

42. def hello()
43.   puts "hello"
44. end
@gasi

This comment has been minimized.

Copy link

gasi commented Jun 20, 2018

@oe and @DirtyF: First of all, thanks for the quick review!

@DirtyF: Quick test without an include: it modifies the line numbers but always start at the beginning of the code snippet.

Good point! That is by design as {% highlight %} is only concerned with presentation and not with how or where the data comes from. Maybe that’s why Pygment’s naming convention of linenostart is clearer — it expresses that it’s just where the line number starts, not at which line the code listing begins to be included (although that’d be nice to solve with a more code-centric include tag).

Given your feedback, I was going to propose adopting linenostart from Pygment. But then you mentioned Pygment will be dropped in 4.0. Also, linenostart is a poor API name for googling. I was trying to find this functionality by searching ’jekyll highlight start line‘, etc. but all that came up was Rouge’s start_line API, hence the choice above.

Ultimately, I’d propose linenostart to align with existing linenos and long-term, when Pygments is dropped in 4.0, add clearer APIs such as:

  • line_numbers (instead of linenos)
  • line_numbers_start (instead of linenostart)

What do you think?

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jun 20, 2018

and long-term, when Pygments is dropped in 4.0, add clearer APIs

I don't believe this feature will be released until 4.0, by which time Pygments will have already been dropped. Therefore, I don't think we need to worry much about Pygments for this PR.

@gasi

This comment has been minimized.

Copy link

gasi commented Jun 20, 2018

@pathawks I don't believe this feature will be released until 4.0, by which time Pygments will have already been dropped. Therefore, I don't think we need to worry much about Pygments for this PR.

Understood. To clarify, I agree that we can drop the implementation support for Pygments above but one thing that will presumably survive the dropping of Pygments is the {% highlight ruby linenos %} API as linenos comes straight from Pygments: http://pygments.org/docs/formatters/

If we wanted to have a consistent API, we’d also use Pygments linenostart naming, but I find that overall to be poor API naming. Since Pygments is dropped, maybe we can alias linenos with line_numbers and adopt start_line straight from Rouge (or line_numbers_start): https://github.com/jneen/rouge/tree/ea7675f741ee28f3f177ff32a9bde192742ffc59#formatters

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Jun 20, 2018

@gasi, not that its a concern but what if start_line is used without linenos option?

@gasi

This comment has been minimized.

Copy link

gasi commented Jun 20, 2018

@ashmaroli: not that its a concern but what if start_line is used without linenos option?

It’s currently a no-op. What’s Jekyll’s philosophy? Should we fail fast?

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Jun 21, 2018

It’s currently a no-op

@gasi I was wondering if you could wire it up to a single option..

{% highlight ruby start_line=42 %} automatically enables the linenos option internally..

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Jul 10, 2018

@oe @pathawks @DirtyF Can we have start_line enable the linenos option internally if its not provided..? Or is that against the no-magic-philosophy?

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jul 10, 2018

@ashmaroli That makes sense to me 👍

@dAmnation69

This comment has been minimized.

Copy link

dAmnation69 commented Jan 1, 2019

is this enhancement available in the latest version ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment