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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include _config.yml in a new theme's gemspec #7865

Merged
merged 3 commits into from Feb 6, 2020

Conversation

@bigelowcc
Copy link
Contributor

bigelowcc commented Oct 18, 2019

This is a 馃悰 bug fix.

Summary

#7304 introduced loading the _config.yml from a theme, but the gemspec doesn't include this file.

Currently a site will correctly include values from the theme's _config.yml locally, but not once the theme is packaged and pulled from a gem repository, as the file doesn't exist.

This PR adds _config.yml to the gemspec generated when you run jekyll new-theme.

Context

#7304 introduced this feature.

Notes

  • I haven't added a test for this change as there currently doesn't seem to be a test around gem packaging
  • All existing themes will still encounter this problem unless they amend their gemspec files to include _config.yml
@ashmaroli ashmaroli added the fix label Oct 18, 2019
@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Oct 18, 2019

Thank you for submitting this @bigelowcc.
I should've foreseen this and documented appropriately. Better late than never.

Regarding tests, we currently cover this aspect via Cucumber features.

As for the proposed change, the feature is currently limited to just loading _config.yml at the root of the theme-gem. _config.yaml will be ignored.

Jekyll 4.0 only loads a `_config.yml` at the root of the theme_dir
@DirtyF
DirtyF approved these changes Dec 23, 2019
Copy link
Member

DirtyF left a comment

LGTM

@DirtyF DirtyF added this to the 4.1 milestone Dec 23, 2019
@DirtyF DirtyF added this to Reviewable in Jekyll 4.1 Dec 23, 2019
@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Feb 6, 2020

Not merging this right away since we need to decide if Jekyll should bundle _config.yml by default.
Theme authors should ideally opt-in voluntarily to bundle the config file in the gem..

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Feb 6, 2020

If you're packaging a theme as a gem, we should make it easiest for the user to use the gem by default. Theme authors should opt-out.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 6097d3b into jekyll:master Feb 6, 2020
7 checks passed
7 checks passed
SUITE: test / OS: ubuntu-latest
Details
SUITE: test / OS: windows-latest
Details
SUITE: default-site / OS: ubuntu-latest
Details
SUITE: default-site / OS: windows-latest
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/jekyllrb/deploy-preview Deploy preview ready!
Details
Jekyll 4.1 automation moved this from Reviewable to Done Feb 6, 2020
jekyllbot added a commit that referenced this pull request Feb 6, 2020
@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Feb 6, 2020

Theme authors should opt-out.

Note to myself: Ensure that this is mentioned in the v4.1.0 release-post since novice users won't realize they're bundling their config file right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Jekyll 4.1
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can鈥檛 perform that action at this time.