Propagate _data folder from theme

[mgerzabek]

All files from _data folder within theme are accessible in consumer project. The behaviour is exactly the same as for overrides of _includes and _layouts. Data files with the same name in consumer project override data files within remote theme.

A similar PR was originally created for @benbalter jekyll-remote-theme which didn’t get the attention of the maintainers.

On 23rd of July @parkr said

I think this could be a really useful feature for all Jekyll themes. Would you consider submitting the change to the Jekyll project upstream?

So this my try on getting it into Jekyll upstream. And hopefully this way I can get what I wanted first, being able to access _data folder content in my remote themes :-)

This is a 🙋 feature or enhancement.

• I didn't add tests because I’m new to ruby and happy enough to got the code working. If someone could point me to where and how to add tests, I’d happily learn this…
• I didn’t adjusted the documentation simply because I’m not sure where to add that piece of documentation. Please point me to the corresponding locations and I’ll do it…
• The test suite passes locally… but, I get the message lib/jekyll/reader.rb:14:5: C: Metrics/AbcSize: Assignment Branch Condition size for read is too high. [<2, 26, 0> 26.08/23] where I don’t really know how to go about that.

Prior Art

• jekyll-data — plugin authored by @ashmaroli.
• Allow reading data files bundled within theme-gems #5470 — prior attempt by @ashmaroli to incorporate this feature in Core.

[ashmaroli]

@mgerzabek Have you tried using the jekyll-data plugin also referenced in your prior pull request?
There was an attempt to add this functionality to Core but I pulled the plug because unlike includes and layouts, data files were not strictly referenced by their filenames.
Data files provide a high degree of flexibility.

Consider the following object: site.data.movies.english. Will you be able to determine with certainty how the object originated?
Was it

• via _data/movies/english{.y(a)ml, .json, .csv, .tsv} or
• via english: key map in _data/movies{.y(a)ml, .json, .csv, .tsv}
  ..?

[mgerzabek]

Have you tried using the jekyll-data plugin also referenced in your prior pull request?

No, as I’d been only been aiming at making it work in junction with jekyll-remote-theme.

So, regarding your question @ashmaroli, I see the ambiguity. How to go about it? Thinking back to my use case I’d be fine to remove the recursive directory walking since it’s good enough to distribute data files within _data alone. This would make your second option the only possible way to contribute site.data.movies.english through themes and remove the ambiguity. What do you think?

[ashmaroli]

I’d been only been aiming at making it work in junction with jekyll-remote-theme.

Please try using the plugins in tandem.

# in _config.yml
plugins:
  - jekyll-remote-theme
  - jekyll-data

to remove the recursive directory walking since it’s good enough to distribute data files within _data alone. This would make your second option the only possible way..

That is a good alternative I had not considered. The plugin I wrote is recursive within themes as well for parity with behavior for data files at source. I see your current implementation is recursive as well.
However, that doesn't mean there's no scope for further developments. If you can improve the implementation, add adequate test coverage, covering various use-case scenarios , I'll reconsider adding to Core.

Do note that if your primary need is for deploying with GitHub Pages, don't have high hopes. GHP is locked to Jekyll 3.9. This feature when ready will only go into Jekyll 4.x

[mgerzabek]

Do note that if your primary need is for deploying with GitHub Pages, don't have high hopes. GHP is locked to Jekyll 3.9. This feature when ready will only go into Jekyll 4.x

I understand and thank you for your clear words!

This makes me rethink my initial considerations that where based on GHP via jekyll-remote-theme being able to also mirror the _data folder from the theme… if there is anyone out there who wants this PR in Jekyll 4+ Core, please vote with +1. If there are more than 50 votes until 30th of September 2021, I’d improve the implementation and add the tests to comply to @ashmaroli requirements.

Otherwise I’d just leave this alone since, as I mentioned previously, I’m new to Ruby and the extra work means a lot of efforts beside my day to day job.

(This BTW somehow makes me feel the original PR for @benbalter jekyll-remote-theme was the right location anyway…)

[ashmaroli]

if there is anyone out there who wants this PR in Jekyll 4+ Core, please vote with +1. If there are more than 50 votes until 30th of September 2021,

@mgerzabek This could sound a bit obtuse, but the chances of interested users seeing the comment above by Sep 30th is very slim. To increase the chances of visibility, you may want to consider utilizing platforms such as Jekyll Talk, Reddit or Twitter.

feel the original PR for jekyll-remote-theme was the right location anyway…

Technically, no. It wasn't the right location.
jekyll-remote-theme is meant to do one thing: Download theme repo from GitHub and allow users to consume the downloaded theme as a local theme.
What Jekyll does with the theme is not the plugin's primary concern.

However, since your primary need is to get this feature working on GHP and jekyll-remote-theme is a plugin revolving around themes, you could make an argument.

I’m new to Ruby and the extra work means a lot of efforts beside my day to day job.

My humble request would be to consider trying to migrate from GHP and use GitHub Actions to deploy your site.
You don't need prior Ruby programming experience for that. Once set up you can just copy your implementation here to a *.rb file inside _plugins folder at source and Jekyll (all versions) will pick it up.

[mgerzabek]

the chances of interested users seeing the comment above by Sep 30th is very slim.

Right, so I‘ll edit the deadline to be 31st of October and do a post on Jekyll Talk

Plz cast your votes until 31st of October 2021. Also feel free to share…

[michaelbach]

+1

[datapolitical]

+1

[parkr]

I'm comfortable reading YAML/JSON from _data/. Any other file types (like icons, css, JS, etc) should be placed in assets/ instead.

Have we decided how we want precedence to work? We should make sure our docs are clear for this, preferably with an example.

👍

[datapolitical]

One of my major use cases is a package of json files that goes along with my theme that is kept in _data/drinks. These get read and processed into pages by Jekyll-datapage-gen.

If you don't want to scan all subfolders recursively for yml/json, you could allow explicit specification of folders to be scanned in _config.yml

[ashmaroli]

@parkr Please refer #8815 (comment) and let us know if the concern can be handled without impacting maintainability

[datapolitical]

@parkr Please refer #8815 (comment) and let us know if the concern can be handled without impacting maintainability

@ashmaroli I guess I’m a little confused, doesn’t that ambiguity already exist with the _data folder itself? Doesn’t exist with the use of your plugin Jekyll-data?

What makes it any worse putting it into core?

[ashmaroli]

Doesn’t exist with the use of your plugin Jekyll-data?

Yes, It does exist with jekyll-data.

What makes it any worse putting it into core?

Having issues with a plugin is one thing. Bringing a known issue into Core is another.
A plugin can be disabled / abandoned (by the user) if it is causing complications. Making changes to the Core comes with lot of overhead (adherence to semver, provide additional feature to enable/disable functionality, interference with other moving parts in Core, affect on users fine with published API, etc..)

[datapolitical]

That makes sense. So if local data always overrides theme I don't see how it would cause confusion.

I do think it makes sense to allow users to be explicit about which folders to include if they want.

In my case, the theme would come with 500 or so files and a user may only want a handful of them.

If they create a _data/drinks folder and place three new yml files inside, default behavior would be to include all the others as well, and some/many may not want that. Giving people a way to say "not this folder" or "only this folder" makes sense.

Fortunately we already have that concept in Jekyll's _config.yml with "exclude:"

[parkr]

I think we should add this feature. I'd love to be able to reduce the differences between the ThemeDataReader and the Jekyll::DataReader classes so we can reuse the logic in the latter.

Tests are super thin for the DataReader class, so it would be great to add some more tests for that class using our fixture theme with some data files. 😄

Do you need help with any of this?

[parkr]

Thanks for your patience on this one! I left fairly detailed comments with changes I'd propose. Let me know if you have any questions! Outstanding items:

1. Write tests
   1. Site without a theme
   2. Site with a theme without _data
   3. Site with a theme with empty _data directory
   4. Site with a theme with data at root of _data, and in a subdirectory
2. Write docs for our website (these can be done in a separate PR, e.g. https://jekyllrb.com/docs/themes/#understanding-gem-based-themes needs to be updated)

[mgerzabek]

I left fairly detailed comments with changes I'd propose.

Thank you @parkr for the Jekyll/Ruby coaching. This was very insightful to me. I love the ambiguation at method level for initializing the reader. This is far advanced stuff I never would think about. Also the guarding of blocks is a rather new concept to me that combined with an unless seems very helpful on language level. Awesome!

As a next step I’ll prepare the tests. After bringing it all to work I’ll open a separate pull request for updating the docs as suggested.

[ashmaroli]

@mgerzabek Like I have stated in the comment above,
cucumber tests are more important for this pull request. Unit tests being secondary.

Since you're lost, I'll guide you towards finishing the Cucumber tests first..

You have already started with a new scenario in theme.feature, Great!
To test that you need to run script/cucumber instead of script/test.

• So, go ahead and and first run script/cucumber features/theme.feature:44. It will pass. (false-positive, though..)
• Now make a change to the string assigned to foo: in the data file within the test-theme fixture.
• Then rerun script/cucumber features/theme.feature:44. Does it fail? Or pass? If your "assumptions" outlined in the scenario earlier today are solid, then the test should've failed..

[mgerzabek]

Thank you @ashmaroli. Now I know how start the cucumber tests… and in the meantime I scanned through the introduction on cucumber on cucumber.io.

If I understand correctly anything that I put into the scenario, like the data file in line 4 will be in the site. So with the greetings.yml already in test/fixtures/test-theme/_data and the site.data.greetings in line 4 the test has to pass in any case, since the contents from the site overwrite the contents from the theme by definition.

  Scenario: A theme with data
    Given I have a configuration file with "theme" set to "test-theme"
    And I have an _data directory
    And I have an "_data/greetings.yml" file with content:
      """
      foo: "Hello! I’m foo. And who are you?"
      """
    And I have an "index.html" page that contains "{{ site.data.greetings.foo }}"
    When I run jekyll build
    Then I should get a zero exit status
    And the _site directory should exist
    And I should see "Hello! I’m foo. And who are you?" in "_site/index.html"

Spoken the other way around when I change the key foo in test/fixtures/test-theme/_data to something different, the test will also succeed. And that is the desired behaviour, isn’t it?

I changed the contents of test/fixtures/test-theme/_data to contain foo: "Hello! I’m bar. What’s up so far?" and made another cucumber.

  Scenario: A theme without data
    Given I have a configuration file with "theme" set to "test-theme"
    And I have an "index.html" page that contains "{{ site.data.greetings.foo }}"
    When I run jekyll build
    Then I should get a zero exit status
    And the _site directory should exist
    And I should see "Hello! I’m bar. What’s up so far?" in "_site/index.html"

Now both scenarios succeed.

[mgerzabek]

Please undelete that test file (Its a symlink needed for testing our {% include ... %} tag)

Okay, done. But counter intuitive to your request ist the last line in .gitignore in project root. It excludes all files within temporary directore (line 28: tmp/*).

When I delete the file, then run the testsuite the file is created anyway. Not sure about that.

[mgerzabek]

Please undelete that test file (Its a symlink needed for testing our {% include ... %} tag)

Arrgh, too fast. Sorry @ashmaroli !

Where does this symlink point to? Do I have to recreate the symlink via terminal or is there a way I could use github to recreate it? I’m not sure how to proceed.

[ashmaroli]

Where does this symlink point to?

@mgerzabek The commit a2b6aa2 and the associated PR has all the info to answer your questions. Give it a try. Don't waste your time though. I'll have a go at it before the final review.

[mgerzabek]

I'll have a go at it before the final review.

@ashmaroli as I don’t get the point, thank you for jumping in!

[ashmaroli]

I'll have a go at it before the final review.

as I don’t get the point...

The point being you were struggling and sometimes, it's easier just doing it instead of giving instructions that the other party can easily follow..
You may sync your local branch by running:

git pull origin theme_w_data_folder

[mgerzabek]

it's easier just doing it instead of giving instructions that the other party can easily follow..

Thank you! Updated my local repo.

So, is there anything left to do? How do we proceed from here? I’m curious how this works out…

[ashmaroli]

So, is there anything left to do?

I'm not so pleased with the documentation. However, I'll defer sorting that and anything remaining to @parkr

Concerns addressed

[mgerzabek]

I'm not so pleased with the documentation.

Well, @ashmaroli if you can tell me what should be improved I‘d jump in… not so pleased is kind of vague though.

Anyways, @parkr said, the docs could be addressed in another PR as well… so why not proceed from here?

[ashmaroli]

if you can tell me what should be improved I‘d jump in…

You did tell us that you're not a native English speaker and hence the docs may not be up to expected level. It so happens that I'm not a native English speaker as well. So, my suggestions to improve the docs may be shot down by a consequent review.

Let us wait for @parkr's inputs. If he doesn't get the time by Tuesday, I'll ping another maintainer.

In the meantime, you could make the following edits (not mandatory, but highly recommended):

• Reduce line-lengths to 120 chars.
• Rephrase everything to third-person voice. (i.e your theme to a theme, she/he to they, et cetra..)
• Use regular punctuation. (i.e. it’s to it's), themes to theme's, et cetra..

[parkr]

This looks great! I think further improvements could be made in a follow-up PR as needed as long as CI returns green.

[ashmaroli]

Thanks @mgerzabek
@jekyllbot: merge +minor

[mgerzabek]

Thanks @ashmaroli!

[parkr]

Well done @mgerzabek and thanks @ashmaroli for the review!