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

Support custom config files from CLI #322

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Mar 12, 2017

Fixes #321

When you provide an array of config files via the --config switch, we'll read and write to the first of such files.

  • add code
  • test code

Copy link
Contributor

@benbalter benbalter left a comment

Choose a reason for hiding this comment

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

What's the desired user experience here? To only be able to modify the first configuration file? Would it make more sense to abstract configuration into configuration files? How often do we think users will want to modify secondary configuration files? IIRC having a singular configuration file was a purposeful design decision to keep things slim (but that doesn't mean it can't be changed now if there's a strong reason).

@@ -2,6 +2,21 @@ module Jekyll
module Commands
class Serve < Command
class << self
def process(opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than monkey patching another method, can we pass the options hash to our existing monkeypatch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find another way to bypass this deletion


def custom_configs
return nil unless @overrides
@overrides
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be simplified to an attr_accessor?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was under the impression that when we define an attr_accessor, its called again via #initialize method

@ashmaroli
Copy link
Member Author

ashmaroli commented Mar 13, 2017

What's the desired user experience here?

Primarily, to be in sync with existing Jekyll functionality and be able to edit the site built/served with the specified configuration file.

--

To only be able to modify the first configuration file? Would it make more sense to abstract configuration into configuration files?

When the user builds/serves via a list of config files, its understood and documented that the effective config hash will be a merged singularity. But instead of displaying contents of all the config files in the list, we shall simply allow writing to the first of specified config files.

--

How often do we think users will want to modify secondary configuration files?

Jekyll's build options allow one to specify custom config files. JekyllAdmin's UX gets hampered when we turn a blind-eye to an existing long-standing core functionality.

--

IIRC having a singular configuration file was a purposeful design decision to keep things slim (but that doesn't mean it can't be changed now if there's a strong reason).

For initial release, that was a wise decision. But its time to expand the scope.. 😁

@ashmaroli ashmaroli force-pushed the custom-config-hash branch from b4b3369 to 918e9b5 Compare April 24, 2017 17:58
@ashmaroli
Copy link
Member Author

I believe this is ready a final review..

@ashmaroli
Copy link
Member Author

Update: This will not work on Jekyll-3.5.0. Expecting refinements from Jekyll itself..

@ashmaroli
Copy link
Member Author

@mertkahyaoglu @benbalter Guys, I believe this is once again ready for a scrutiny..
I temporarily disabled a Rubocop check to have it handled as required in a separate PR..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants