Allow Jekyll Source Directory #42

mloberg · 2016-09-01T14:18:33Z

Jekyll Compose assumes that the Jekyll code lives in the root directory, so if you have custom source set in your config, it won't respect that.

I've updated compose to load in configuration (adding both --config and --source options).

In some Jekyll setups, the site assets are in a sub-directory. The drafts command, does not process any configuration files or allow to set the site source. Add these options to the draft command and update the FileCreator class to take an optional root that will be prepended to the file.path.

Load in Jekyll configuration when parsing args. Add source method to return Jekyll source directory and update the drafts command to use this new method.

Update the page command to use the correct Jekyll source from config or command line option.

Update post command to respect any custom Jekyll source.

Update Publish command to allow custom Jekyll source. Update MovementArgParser to generate Jekyll config like was done with ArgParser. Update FileMover to automatically use a root directory if given.

Update Unpublish command to allow custom Jekyll source.

Both Publish and Unpublish take in the relative path as the argument, so prepending the root directory won't give you the correct path to the file. Just prepend the root directory to the target path.

parkr · 2016-09-01T20:59:34Z

Seems logical to me. LGTM.

@jekyll/compose?

courajs · 2016-09-09T15:10:32Z

lib/jekyll-compose/arg_parser.rb

@@ -24,4 +25,8 @@ def title
  def force?
    !!options["force"]
  end
+
+  def source
+    source = config['source'].gsub(/^#{Regexp.quote(Dir.pwd)}/, '')


Is this variable assign necessary?

Other methods had an assign, so I thought I would follow that pattern. I can remove this if it isn't doing anything.

Oh I hadn't noticed that. I'm pretty confident those do nothing, would you remove all three?

Yes. I'll update that tonight.

I've updated this and removed all unnecessary variable assignments in that class.

courajs · 2016-09-09T15:23:28Z

Great work! My only feedback would be that the specs are becoming hard to read, because of the overriding of lets. For example in draft_spec.rb, the body of it 'should use source directory set by config' is exactly the same as the simple case it 'creates a new draft', but it's different because drafts_dir is overridden down here, which causes path to evaluate differently at the top.

If you have the time to refactor that to make it clearer, that would be amazing! If not, this is mergable as-is.

LGTM

mloberg · 2016-09-09T15:59:10Z

@courajs I haven't done much Ruby and almost no spec testing in Ruby. Are there any guides or best practices that might help me out?

courajs · 2016-09-09T18:39:31Z

I'm not sure there's a guide that would help you with this specific issue. But I think it will make things clearer to explicitly re-define path, rather than re-defining drafts_path and relying on changes to cascade. Then make it similarly explicit for post_path and draft_path in publish_spec.rb and unpublish_spec.rb.
If necessary we can always merge as-is, and I'll open a refactoring issue for someone interested to pick up.

The variable assignments in ArgParser do nothing, so remove them.

mloberg · 2016-09-10T02:42:33Z

@courajs Is something like this what you're looking for?

it 'creates draft in specified source directory' do
  site_dir = 'site'
  path = Pathname.new source_dir(site_dir, '_drafts', 'a-test-post.md')
  expect(path).not_to exist
  capture_stdout { described_class.process(args, 'source' => site_dir) }
  expect(path).to exist
end

courajs · 2016-09-10T15:01:13Z

Ah, I didn't realize the after(:each) hook was using draft_dir to clean up. It's seeming like a lot of effort for not too much gain, I'll just merge this. The CI failure is an intermittent problem that I opened an issue for (#44), but they pass locally for me.

mloberg and others added 7 commits September 1, 2016 08:27

Refactor ArgParser to load Jekyll configuration

d7bb9af

Load in Jekyll configuration when parsing args. Add source method to return Jekyll source directory and update the drafts command to use this new method.

Add source and config options to page command

7b71ac4

Update the page command to use the correct Jekyll source from config or command line option.

Add source and config options to post command

d9469c8

Update post command to respect any custom Jekyll source.

Add source and config options to publish command

9103444

Update Publish command to allow custom Jekyll source. Update MovementArgParser to generate Jekyll config like was done with ArgParser. Update FileMover to automatically use a root directory if given.

Add source and config options to unpublish command

d20fb11

Update Unpublish command to allow custom Jekyll source.

Fix FileMover

696a74c

Both Publish and Unpublish take in the relative path as the argument, so prepending the root directory won't give you the correct path to the file. Just prepend the root directory to the target path.

courajs reviewed Sep 9, 2016
View reviewed changes

Remove unneeded variable assignment in ArgParser

d467f20

The variable assignments in ArgParser do nothing, so remove them.

courajs merged commit 8100f7e into jekyll:master Sep 10, 2016

jekyll locked and limited conversation to collaborators Apr 23, 2019

jekyllbot added the frozen-due-to-age label Apr 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Jekyll Source Directory #42

Allow Jekyll Source Directory #42

mloberg commented Sep 1, 2016

parkr commented Sep 1, 2016

courajs Sep 9, 2016

mloberg Sep 9, 2016

courajs Sep 9, 2016

mloberg Sep 9, 2016

mloberg Sep 10, 2016

courajs commented Sep 9, 2016

mloberg commented Sep 9, 2016

courajs commented Sep 9, 2016

mloberg commented Sep 10, 2016

courajs commented Sep 10, 2016 •

edited

Loading

Allow Jekyll Source Directory #42

Allow Jekyll Source Directory #42

Conversation

mloberg commented Sep 1, 2016

parkr commented Sep 1, 2016

courajs Sep 9, 2016

Choose a reason for hiding this comment

mloberg Sep 9, 2016

Choose a reason for hiding this comment

courajs Sep 9, 2016

Choose a reason for hiding this comment

mloberg Sep 9, 2016

Choose a reason for hiding this comment

mloberg Sep 10, 2016

Choose a reason for hiding this comment

courajs commented Sep 9, 2016

mloberg commented Sep 9, 2016

courajs commented Sep 9, 2016

mloberg commented Sep 10, 2016

courajs commented Sep 10, 2016 • edited Loading

courajs commented Sep 10, 2016 •

edited

Loading