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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cascade problem with site.baseurl. #935

Merged
merged 2 commits into from
Apr 7, 2013
Merged

Conversation

parkr
Copy link
Member

@parkr parkr commented Apr 7, 2013

Problem described in #931. This is a fix for it.

@brainkim
Copy link
Contributor

brainkim commented Apr 7, 2013

What a coincidence! I was digging around the code and saw this problem while mapping out the options and how they transform/cascade throughout the program (it's complex). The problem is that any defaults we define with commander will always override Jekyll::DEFAULTS as well as _config.yml, which is how Jekyll.configuration was initially designed. My recommendation (I am not a committer so take this with a grain of salt) would be to move all the Commander defaults except :serving to Jekyll::DEFAULTS so that we have them all in one place and don't have any surprising behavior/discrepancies. Removing baseurl won't be enough because we've also lost the ability to set the port and host using _config.yml. :serving currently makes the build and serve commands play nice but could also be refactored out as well.

@parkr
Copy link
Member Author

parkr commented Apr 7, 2013

@mojombo would like to keep all options in Jekyll::DEFAULTS.

parkr added a commit that referenced this pull request Apr 7, 2013
Fix cascade problem with site.baseurl.
@parkr parkr merged commit b667a6a into master Apr 7, 2013
@parkr parkr deleted the baseurl-normalization branch April 7, 2013 22:19
parkr added a commit that referenced this pull request Apr 7, 2013
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants