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 docs server options override #1444

Merged
merged 4 commits into from Aug 24, 2013

Conversation

Projects
None yet
5 participants
@chrisnicotera
Contributor

chrisnicotera commented Aug 22, 2013

Override the source and destination before passing it along to the
configuration

Fix docs server options override
Override the source and destination before passing it along to the
configuration
@kelvinst

This comment has been minimized.

kelvinst commented Aug 22, 2013

I revised, and will make considerations into notes.

But I have another question: There is some reason that I don't get to do this? The old way doesn't work or is slow?

bin/jekyll Outdated
@@ -122,10 +122,11 @@ command :docs do |c|
c.action do |args, options|
options = normalize_options(options.__hash__)
options = Jekyll.configuration(options).merge({
options = options.merge({

This comment has been minimized.

@kelvinst

kelvinst Aug 22, 2013

To make this, you can do:

options.merge!({
  new_options
})

This way able you to do just:

options = Jekyll.configurations(options.merge!({
  new_options
}))

And, personally, I do not like these multiple attributions to the same variable, further when the two attributions are of different kinds. I don't really understand why I do not like this, since this, for me, is a pretty nice feature of ruby language (the duck typing). Maybe some code addiction of mine, or maybe I've fear of what this can become.

In short, I'd do it this way:

options = Jekyll.configurations(normalize_options(options.__hash__).merge!({
  new_options
}))

This comment has been minimized.

@chrisnicotera

chrisnicotera Aug 22, 2013

Contributor

I wrote it that way mostly for readability and in order to follow the convention of what was there already. I have no particular affinity to how it's written as long as it works. Feel free to go ahead and modify as you deem fit.

This comment has been minimized.

@kelvinst

kelvinst Aug 22, 2013

OK... But about the merge!, it would be better if you use it, it's pretty more fast and use less memory, since the merge returns a merged new copy of the hash, and merge! just merge in the existent instance.

@chrisnicotera

This comment has been minimized.

Contributor

chrisnicotera commented Aug 22, 2013

Sorry, I'm not sure that I understand this:

But I have another question: There is some reason that I don't get to do this? The old way doesn't work or is slow?

The reason for my pull request was that the configuration was reading the config files:

config = config.read_config_files(config.config_files(override)) # lib/jekyll.rb, line 74

and then merging in the new source and destination. This is fine except for the fact that it skips reading the _config.yml from the source directory, which was causing the docs site to function improperly. Hope this explains it!

@kelvinst

This comment has been minimized.

kelvinst commented Aug 22, 2013

Nice, totally explained now. 😃

@chrisnicotera

This comment has been minimized.

Contributor

chrisnicotera commented Aug 22, 2013

I updated the pull request to use merge!, but not using your preferred way of writing it just to maintain consistency with the other methods. Thanks for the tip!

@kelvinst

This comment has been minimized.

kelvinst commented Aug 22, 2013

Of course @chrisnicotera. About the other way to write it, as I said before, it's just an opinion.
@parkr or @mattr-, totally revised, it would be nice to have this correction soon ;D

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

Sure, this LGTM. @mattr-?

@mattr-

This comment has been minimized.

Member

mattr- commented Aug 24, 2013

Is this for the local serving of the docs? Would you mind going into a little bit more detail about what this does and why it's needed?

@mattr-

This comment has been minimized.

Member

mattr- commented Aug 24, 2013

Ah, never mind. just saw the other comment. Thanks for the patch! 💛

mattr- added a commit that referenced this pull request Aug 24, 2013

Merge pull request #1444 from chrisnicotera/master
Fix docs server options override

@mattr- mattr- merged commit 1b40226 into jekyll:master Aug 24, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Aug 24, 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.