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

bglw
Copy link
Contributor

@bglw bglw 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
Copy link
Member

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
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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-
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
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
Copy link
Member

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.

@jekyll jekyll locked and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Jekyll 4.1
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants