Process all pages and don't Liquid-render pages corpus

18F / jekyll_pages_api

a Jekyll Plugin that generates a JSON file with data for all the Pages in your Site

Other

43 stars 14 forks source link

Process all pages and don't Liquid-render pages corpus #24

Closed mbland closed 9 years ago

mbland commented 9 years ago

This is really two PRs in one, motivated by my work to build 18F/jekyll_pages_api_search. First, it adds Posts, Documents, and StaticFiles to the pages corpus. Second, it prevents the resulting pages.json corpus from being rendered by Liquid.

I noticed the need for these features when experimenting with interating the jekyll_pages_api_search plugin into 18F/18f.gsa.gov. The absence of Posts from the corpus made the search less useful; then the presence of example Liquid markup in the Jekyll and webhooks post caused the pages.json file to be invalid JSON due to rendering the corpus with Liquid, rather than just assigning the JSON output directly to page.output.

With these changes, I'm able to build and integrate search indexes both into 18f.gsa.gov and the 18F/hub.

cc: @afeld

mbland commented 9 years ago

cc: @adelevie, since Aidan's mostly out this week.

adelevie commented 9 years ago

LGTM, merging in 30 minutes.

mbland commented 9 years ago

@adelevie Just added a "Developing" section to the README. If it looks sufficient, I'll add a similar section to 18F/jekyll_pages_api_search#2.

afeld commented 9 years ago

Not sure why I was so nitpicky on this one, sorry! Looks really good overall. A few overall comments:

We might want to add something to the README saying what types of Documents will and won't be included in the API.
Not sure about including all Document types in the pages.json (cc #2)...thoughts?
FYI, a Rubyism is to use something.nil? rather than something == nil. Not sure why.

Thanks!

mbland commented 9 years ago

OK @afeld, just push a commit addressing all your comments from above. I updated the README to mention that all Pages, Posts, Documents and StaticFiles are processed. It's possible we may want to leave out StaticFiles, but if they happen to contain HTML, why not include them? Also, I like the .nil? notation; very convenient and compact. As always, I learn all these little idioms that make me a better Rubyist from your reviews. :-)