Skip to content
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

Refactor `highlight` tag to behave like the `raw` tag #6821

Merged
merged 12 commits into from Mar 15, 2019

Conversation

Projects
6 participants
@ashmaroli
Copy link
Member

commented Mar 2, 2018

Closes #6658

With this, the highlight tag will no longer parse Liquid within the tag-body.

{% highlight ruby %}
{{ site.title }}
def print_hi(name)
  puts "Hi, #{name}"
end
print_hi('Tom')
#=> prints 'Hi, Tom' to STDOUT.
{% endhighlight %}

will produce

{{ site.title }}
def print_hi(name)
  puts "Hi, #{name}"
end
print_hi('Tom')
#=> prints 'Hi, Tom' to STDOUT.
  • initial implementation
  • update tests
  • update documentation

@ashmaroli ashmaroli added this to the 4.0 milestone Mar 2, 2018

Show resolved Hide resolved lib/jekyll/tags/highlight.rb Outdated
@pathawks

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

This will of course make {% highlight ruby %}{% include example.rb %}{% endhighlight %} impossible, but I agree that the trade is probably worth it.

A backwards-compatible alternative would be something like a {% highlight-raw %} tag.

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2018

This will of course make {% highlight ruby %}{% include example.rb %}{% endraw %} impossible..

How is that syntax even valid??
To achieve what you meant, I'd use the backticks-fenced-block.. though I won't be able to print linenos

--

A backwards-compatible alternative would be something..

I have a backwards-compatible alternative implementation locally.. but it is a new tag inheriting from Tags::HighlightBlock and involves some duplicate code.. I feel, library-wise this is the cleanest move..

@pathawks

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

I feel, library-wise this is the cleanest move

I agree 💯

@pathawks pathawks requested review from jekyll/stability Apr 30, 2018

@ashmaroli ashmaroli added this to In progress in Jekyll 4.0 May 10, 2018

@pathawks
Copy link
Member

left a comment

I love this. This will make the highlight tag work more like users expect.

We need some documentation for this; this is a breaking change, so we need to make sure the behavior is explained.

@DirtyF DirtyF added needs-documentation and removed friction labels Jun 23, 2018

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2018

We need some documentation for this...

@pathawks Definitely. Its already part of this PR's todo-list.
But weren't we adding documentation for v4.0 to a separate branch that gets published after v4.0 has been shipped..? Or did that change?

Also I'm waiting for #6983 to be merged first as resolving conflicts here is a lot cleaner..

@ashmaroli ashmaroli force-pushed the ashmaroli:highlight-meets-raw branch from c96bbac to abd5587 Sep 27, 2018

@DirtyF DirtyF added the needs-tests label Nov 13, 2018

Post install message added

@DirtyF DirtyF changed the title WIP: Refactor `highlight` tag to behave like the `raw` tag Refactor `highlight` tag to behave like the `raw` tag Nov 14, 2018

@ashmaroli ashmaroli moved this from In progress to Reviewable in Jekyll 4.0 Dec 26, 2018

@DirtyF

DirtyF approved these changes Mar 15, 2019

@DirtyF

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

@jekyllbot: merge +major

@jekyllbot jekyllbot merged commit 36404b9 into jekyll:master Mar 15, 2019

4 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

Jekyll 4.0 automation moved this from Reviewable to Done Mar 15, 2019

@jekyllbot jekyllbot added the feature label Mar 15, 2019

jekyllbot added a commit that referenced this pull request Mar 15, 2019

@ashmaroli ashmaroli deleted the ashmaroli:highlight-meets-raw branch Mar 15, 2019

@kvz

This comment has been minimized.

Copy link

commented Mar 26, 2019

Sorry for the amateur question, but I've been looking at 4.0 (btw very nice work on that release! 🎉 ) and how would one go about doing {% highlight ruby %}{% include example.rb %}{% endhighlight %} now? I've successfully changed a few occasions in my site's codebase to triple backtick fenced blocks in markdown files, but those blocks do not work in html files it seems.

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

@kvz That is a situation I hadn't considered..
Markdown documents allow interpolation via kramdown by using markdown="1" on the parent HTML container but that doesn't work for HTML source files..

Hmm.. Can you file a bug report? If we can't come up with a patch in time for Jekyll 4.0 release, this feature will be reverted.

Thank you for bringing this to my notice. 👍

@mattr-

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

I'm not sure how @kvz's situation is any different that the exact situations that are called out in the PR body and the feature tests. We've called out that doing the exact example {% highlight ruby %}{% include example.rb %}{% endhighlight %} won't work anymore. The file type doesn't matter.

I wouldn't call this a workaround per se, but I think the same effect could be gotten by having a file, let's call it foo.example with the code:

{% highlight ruby %}
my_fancy_ruby_code_goes_here
{% endhighlight %}

and then doing

{% include foo.example %}

in a post.

@kvz

This comment has been minimized.

Copy link

commented Mar 26, 2019

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

I'm not sure how @kvz's situation is any different that the exact situations that are called out in the PR body and the feature tests.

@mattr- The feature tests do not test the scenario put forth by @kvz. (Using foo.html as a source file which won't be converted by kramdown (? or does it?) and hence use of triple backticks within foo.html won't be converted by kramdown.)
Moreover, the tests do not suggest a what will work. Just what won't work. (My bad!)

However, I like your suggestion. So basically, we reverse the operation.
Where one would highlight an included snippet in v3.x, in 4.0, they'd be including highlighted snippets. That has to documented in the upgrading post..

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

@kvz Can I see a sample repository? So that I can visualize how it all comes together currently..?

@kvz

This comment has been minimized.

Copy link

commented Mar 27, 2019

Hi, so the project I'm talking about is the site for https://transloadit.com. We'll use code snippets in our docs, demos, and blog posts such as this one https://transloadit.com/blog/2019/03/audio-delay/ that are executed, as well as displayed. It's nice that way as we don't have to keep 2 copies of source files, as over time, they'd certainly diverge, and what you see, has less and less to do with what you get.

For blog posts it's not so much a problem as we can use triple backticks there, but for the demos especially, or the homepage, those are more layout-heavy and as such html files, and we wouldn't be able to upgrade to Jekyll 4 without duplicating our source files as I now understand it, where 1 has the code, and the other has the code, plus {% highlight ruby %}...{% endhighlight %} wrappers, and we'd have to include the latter and run the former.

The repo isn't open source, so i can't share it in its entirety. But the relevant snippets I already shared higher up in the thread, I think. If you need more specifics I'm happy to provide them however.

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

@kvz I understand your situation but unfortunately do not see a clean solution for it.
@mattr- Should we revert this change and introduce a new {% highlight_raw %} tag instead?

ashmaroli added a commit that referenced this pull request Mar 28, 2019

@mattr-

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

@ashmaroli yup, that's the right thing to do. Thanks for opening that revert PR.

Since we'll be introducing a new tag, we can even delay the new tag addition until 4.1 if necessary. Also, I think it makes more sense to call the tag raw_highlight rather than highlight_raw but I'm not attached to any particular name for the tag.

Kiku-git added a commit to Kiku-git/jekyll that referenced this pull request Mar 31, 2019

Kiku-git added a commit to Kiku-git/jekyll that referenced this pull request Mar 31, 2019

ashmaroli added a commit that referenced this pull request Apr 19, 2019

Revert "Refactor `highlight` tag to behave like the `raw` tag" (#7592)
* Revert "Refactor `highlight` tag to behave like the `raw` tag (#6821)"

This reverts commit 36404b9.

* use Liquid `raw` in upgrading document
* let the minor improvements stay
* Revert entry in History.markdown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.