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

Added support for custom source folders! #429

Closed
wants to merge 8 commits into from
Closed

Added support for custom source folders! #429

wants to merge 8 commits into from

Conversation

DougBeney
Copy link

@DougBeney DougBeney commented Nov 13, 2017

Jekyll-Admin has a bug where if users specify a custom source folder in their _config.yml, Jekyll-Admin will actually search in the source file for a _config.yml file.

This code fixes that.

The solution was found by modifying lib/jekyll-admin/path_helper and tweaking the sanitized_path function to support having (and not having) a source folder set in your _config.yml.

What this code does is create a global variable of $PROJECT_SOURCE. This variable will be blank when the user doesn't define a source, and when a user does define a source, it will be equal to that source.

Then, in the server configuration.rb file, I added a regex that will remove that source file from the config location path, since the _config.yml will always be located in the project root.

@ashmaroli
Copy link
Member

I don't see the need to create global variables.. Jekyll Admin uses Jekyll objects so setting custom source directories are handled by Jekyll.

A known issue is that custom config files are not picked up by Jekyll Admin as of current release but there's an open PR to fix that..

@DougBeney
Copy link
Author

The global variable is used because there seem to be other places where that config path is mentioned and having a global variable could come in handy for that sense.

For example, there is still a bug I'd like to fix. When I visit the /admin/configuration URL, a message gets printed to my terminal saying Configuration file: none.

Couldn't find in the code where that is coming from. Any ideas?

And which PR are you referring to? #322? If so, that doesn't look like as seamless of a solution.

@ashmaroli
Copy link
Member

Configuration file: none

That comes from Jekyll directly when you run jekyll serve from outside a Jekyll project directory. In that case you'd need to either cd into the project directory or pass the source-directory path via the --source switch:

$ jekyll serve --source path/to/project/dir

@DougBeney
Copy link
Author

DougBeney commented Nov 13, 2017

When I initially start the server, everything is fine and my configuration is set properly. It's when I visit that /admin/configuration URL, that particular message pops up in my terminal.

So I guess I need to look for the part of the code where Jekyll-Admin reloads the configuration.

Perhaps something to do with lines 16 and 34 of configuration_spec.rb?

Other than this, which I plan to fix, everything else appears to be working flawlessly with this PR.

I'm not familiar with travis-ci and the check that failed, so I will look into that.

@ashmaroli
Copy link
Member

Perhaps something to do with lines 16 and 34 of configuration_spec.rb?

That is a test file. It has nothing to do with how the plugin works..

@DougBeney
Copy link
Author

Yep. Found out it is because I needed to use %r around my regular expressions. Easy fix.

@DougBeney
Copy link
Author

I have some new founding and am working on committing a new version. I've taken your advice about not using a global variable.

@DougBeney
Copy link
Author

Ok, so everything is all set now! Found the solution in path_helper.rb. In my latest commit, I reset the files I previously modified.

Here is the one file I changed. Issue is now fixed! Hope this could get accepted. :) c9f5e51#diff-89f925701193500bd5fd98c1514daa3e

@ashmaroli
Copy link
Member

I still doubt this patch is going to be accepted.. Did you try the command format I gave you earlier? Does that solve your issue without all the regex hacks..?

If you take a look at the CI log, you'll see that this patch breaks many of the existing tests..

@DougBeney
Copy link
Author

I ran your command and it did work, but in my opinion, having to run that --config parameter every single time I want to run my server is a lot hackier.

I wouldn't consider the code I contributed "hacky" either. There is one function. It returns the name of the user-defined source folder. If there is none, of course, a blank string is provided. Then, that project path is removed from the outputted path that sanitized_path returns.

I will take a closer look at why travis-ci is failing.

This change is a big step-up for anyone that uses custom source folders.

@ashmaroli
Copy link
Member

For the last time, setting a source directory is a Jekyll thing.

# lib/jekyll/site.rb

    # Public: Initialize a new Site.
    #
    # config - A Hash containing site configuration details.
    def initialize(config)
      # Source and destination may not be changed after the site has been created.
      @source          = File.expand_path(config["source"]).freeze
      @dest            = File.expand_path(config["destination"]).freeze
[...]

And Jekyll Admin is just a plugin that piggy-backs on an existing instance of Jekyll::Site (via JekyllAdmin.site).. and its a Jekyll convention that there be a config file at the root of the site's configured source

If you feel overburdened to use the --source switch continuously, the easiest and cleanest alternative is to use Shell scripts

#!/bin/sh

set -e

cd path/to/project/directory
bundle exec jekyll serve

@DougBeney
Copy link
Author

DougBeney commented Nov 14, 2017

Completely understand what you've been saying and I appreciate the insight. The problem though is that this plugin should just recognize where the _config.yml is by default. It is not intuitive to the end user to have to modify their terminal command. I love all the great work that has already been put into this plugin, so I'm happy to keep working on this to find a proper solution.

By your shell script solution, I don't believe you fully understand the issue.

Here is a typical file-structure using a source directory:

├── _config.yml
├── Gemfile
└── src
    ├── about.html
    ├── _includes
    ├── index.html
    ├── _layouts
    └── _posts

Here is the config.yml

source: src

I would run jekyll serve in the root directory, where the config file is. Jekyll-Admin searches for the config file in the src/ folder. It shouldn't do that. It should instead search in the root directory of the project, not source.

@DougBeney DougBeney closed this Jan 17, 2018
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.

2 participants