inukshuk / jekyll-scholar

jekyll extensions for the blogging scholar
MIT License
1.13k stars 101 forks source link

Allow newer versions of Jekyll #273

Closed stevecheckoway closed 5 years ago

stevecheckoway commented 5 years ago

This gem works with at least versions 3 and 4. I don't know if it's better to be explicit about limiting to those versions or just allowing any version that's at least 3.

inukshuk commented 5 years ago

Once we are confident that Jekyll 4 works fine, we'll release this with a new major number ourselves and require ~> 4 -- that way you can stay on jekyl-scholar 5.x if you want to use jekyll 3 and jekyll-scholar 6.x for jekyll 4.

stevecheckoway commented 5 years ago

Those look like real issues. (I think I figured out why my local testing didn't catch that: I forgot to run bundle update.)

My current best guess is that in Jekyll 3, there was a slightly different ordering for liquid rendering. I can't find anything in the ChangeLog to this effect and comparing the debug logging for running rake with Jekyll 3 vs. 4 doesn't reveal anything.

It looks like this may be the issue:

        if entry.field_names(config['bibtex_skip_fields']).empty?
          e['bibtex'] = entry.to_s({ quotes: config['bibtex_quotes'] })
        else
          tmp = entry.dup

          config['bibtex_skip_fields'].each do |name|
            tmp.delete name if tmp.field?(name)
          end

          e['bibtex'] = tmp.to_s({ quotes: config['bibtex_quotes'] })
        end

        if raw_bibtex?
          e['bibtex'] = "{%raw%}#{e['bibtex']}{%endraw%}"
        end

This is setting entry.bibtex to be the string version of the entry. If there are liquid tags, those appear verbatim. If the raw option was set, then the {%raw%}/{%endraw%} are appended.

This code makes me think that the original intent was that the liquid markup would be rendered somewhere else in the course of rendering the page. But I don't see where that would happen. The details template will be rendered and as part of liquid rendering, {{ page.entry.bibtex }} will be replaced by the contents, verbatim.

I think what needs to happen is when raw_bibtex? returns false, e['bibtex'] should be replaced by doing liquid rendering. Otherwise, it should not be modified.

I think this PR should be reverted until this is fixed.

stevecheckoway commented 5 years ago

One complicating factor, it's not entirely clear which data should be exposed to via liquid. The function reference_data is used in two places. For details pages, it seems fairly straight forward. There's nothing that could be exposed other than site, entry, and anything appearing in the YAML front matter of the details template.

The other place it is used is in the bibliography. That could expose everything defined in the page.

I'm curious if this is actually a useful feature to expose to users. I can't think of any time I would want liquid markup inside my BibTeX entries.

Looking at one of the examples that's failing,

      @book{a,
        title     = {{'b' | prepend: 'a'}}
      }

it's not clear what this should actually do. BibTeX would parse the title field as having the value {'b' | prepend: 'a'}. Using this BibTeX entry in jekyll-scholar would give (I think) that same string as the value of entry.title which wouldn't be treated as Liquid markup, even if Liquid were given another chance to run on it.

If Liquid rendering were done on the e['bibtex'], then entry.bibtex would produce

      @book{a,
        title     = ab
      }

which isn't valid BibTeX (unless @string { ab = "…" } appeared before it). This could be fixed with title = "{{'b' | prepend: 'a'}}", I suppose.

But even then, what is something like

@misc{a,
  title = "{{ 'b' | prepend: 'a' }}",
  note = "{{ page.entry.title }}",
}

supposed to do in a detail page?

My suggestion is to simply not support rendering Liquid markup in the BibTeX entries at all. Or, at the very most, in the reference_data function, perform Liquid rendering with the site variable available but nothing else.

stevecheckoway commented 5 years ago

Sorry, no, that last suggestion (Liquid rendering in reference_data) doesn't make sense because the rendering would happen after the BibTeX parsing happens. Instead, Liquid rendering should happen when the .bib files are read in Jekyll::Scholar::Utilities#bibliography. And presumably only if they have front matter (which could be exposed to Liquid, I suppose).

inukshuk commented 5 years ago

My suggestion is to simply not support rendering Liquid markup in the BibTeX entries at all.

Yes, I agree.

I'm pretty sure the motivation for introducing the raw_bibtex option was because the generated content in the detail pages was run through liquid by jekyll. This made it impossible, to embed the full bibtex entry verbatim in the page (e.g., if you had double braces in the bibtex somewhere, like title = {{t}he title}). If the liquid rendering does not happen anymore, we may actually be able to drop the raw bibtex option as well?

JoostvanPinxten commented 5 years ago

Indeed, I introduced the raw_bibtex option to support the double brace escape notation in Bibtex, that conflicts with the Liquid double brace syntax. The example from Sylvester actually parses correctly. It was entries such as title={{KEEP capiTALIzation}} that would lead to empty entries.

I concur that there aren't many interesting use cases to support Liquid rendering within bibtex entries. Although, someone might have actually used this.

Anyway, I am in favor of removing the raw_bibtex flag, as the current implementation is not really complete, and leads to some strange fringe cases, depending on what page the entries are rendered.

stevecheckoway commented 5 years ago

I'll put together a new PR that requires Jekyll 4 and removes raw_bibtex (with a warning if someone uses it). If someone is actually using Liquid markup with Jekyll-scholar, then hopefully they'll file an issue and a new implementation could be evaluated based on actual use cases.

inukshuk commented 5 years ago

@stevecheckoway @JoostvanPinxten thanks this looks good to me now.

I'm not using jekyll-scholar myself anymore so the tests are really the minimal quality assurance I have in place, but given that this is by no means a critical Gem, I'll push this with a new major version. That way people can start using it with Jekyll v4 -- should there be any major issues, we can address them later (and users can always pin the previous release, since we did not really add features anyway).