Add show_dir_listing option for serve command and fix index file names #4533

Merged
merged 1 commit into from Mar 24, 2016

Conversation

Projects
None yet
4 participants
@hgoodman
Contributor

hgoodman commented Feb 17, 2016

This is a replacement for PR jekyll/jekyll#4503, which I will close momentarily.

This PR adds the --show-dir-listing CLI option, which can also be included in the config as show_dir_listing, to show the WEBrick default directory listing instead of loading an index file (e.g. index.html). When testing, I find it much more convenient to be able to navigate to a specific file by clicking through a directory list instead of typing in the file name (which may not be linked from index.html).

This PR also fixes the default directory index file list to maintain parity with the file names supported by GitHub Pages (I tested to verify that the order is correct).

I changed the negative option --no-directory-index from PR jekyll/jekyll#4503 to the affirmative --show-dir-listing for the sake of clarity and also because I noticed that GitHub Pages gives its own 404 page when an index file is not present.

@hgoodman

This comment has been minimized.

Show comment
Hide comment
@hgoodman

hgoodman Feb 17, 2016

Contributor

Well, this is awkward (tests failing, I mean). I'm pretty sure it's caused by the execution order since changing the random seed makes it pass and setting it to the value that Travis used makes it fail. My first look at the problem was unfruitful but I'll give it another shot.

Contributor

hgoodman commented Feb 17, 2016

Well, this is awkward (tests failing, I mean). I'm pretty sure it's caused by the execution order since changing the random seed makes it pass and setting it to the value that Travis used makes it fail. My first look at the problem was unfruitful but I'll give it another shot.

@hgoodman

This comment has been minimized.

Show comment
Hide comment
@hgoodman

hgoodman Feb 17, 2016

Contributor

The latest commit emulates GitHub Pages by returning an error when trying to list directory contents in WEBrick unless it's made clear that the user intends to override that behavior in a dev environment with the --show-dir-listing option.

There's that, plus I kinda wanted to take another roll of the dice with Travis CI. 😉

Contributor

hgoodman commented Feb 17, 2016

The latest commit emulates GitHub Pages by returning an error when trying to list directory contents in WEBrick unless it's made clear that the user intends to override that behavior in a dev environment with the --show-dir-listing option.

There's that, plus I kinda wanted to take another roll of the dice with Travis CI. 😉

lib/jekyll/commands/serve.rb
WEBrick::Config::FileHandler.merge({
- :FancyIndexing => true,
+ :FancyIndexing => opts['show_dir_listing'] ? true : false,

This comment has been minimized.

@parkr

parkr Feb 17, 2016

Member

Shouldn't this be flipped? Only do fancy indexing if show_dir_listing is false?

@parkr

parkr Feb 17, 2016

Member

Shouldn't this be flipped? Only do fancy indexing if show_dir_listing is false?

This comment has been minimized.

@hgoodman

hgoodman Feb 17, 2016

Contributor

No @parkr, a "fancy" index in WEBrick-speak turns out to mean showing the contents of the directory when no index is present, which is what I'm trying to do. This is affirmed by the tests.

Anyway, @envygeeks made a good point about not changing behavior for a point release so I was planning to revert it anyway, so as not to hold up the merge. I thought of it as more of a bug fix based on what you were saying about parity with GHP. You decide.

@hgoodman

hgoodman Feb 17, 2016

Contributor

No @parkr, a "fancy" index in WEBrick-speak turns out to mean showing the contents of the directory when no index is present, which is what I'm trying to do. This is affirmed by the tests.

Anyway, @envygeeks made a good point about not changing behavior for a point release so I was planning to revert it anyway, so as not to hold up the merge. I thought of it as more of a bug fix based on what you were saying about parity with GHP. You decide.

This comment has been minimized.

@envygeeks

envygeeks Feb 17, 2016

Contributor

@hgoodman FancyIndexing by default was a conscious disparity between production and development to enforce WEBrick only being dev quality (as used by us) and to discourage the many users we (or maybe just me?) heard about using it in production. People dislike directory indexing in production unless it's for a download folder which helps us let them know this is not meant for production by any means.

@envygeeks

envygeeks Feb 17, 2016

Contributor

@hgoodman FancyIndexing by default was a conscious disparity between production and development to enforce WEBrick only being dev quality (as used by us) and to discourage the many users we (or maybe just me?) heard about using it in production. People dislike directory indexing in production unless it's for a download folder which helps us let them know this is not meant for production by any means.

This comment has been minimized.

@hgoodman

hgoodman Feb 17, 2016

Contributor

Right on, @envygeeks, I don't like it in production either and as I said, I'm planning on reverting it later.

@hgoodman

hgoodman Feb 17, 2016

Contributor

Right on, @envygeeks, I don't like it in production either and as I said, I'm planning on reverting it later.

@parkr parkr added the enhancement label Feb 17, 2016

@hgoodman

This comment has been minimized.

Show comment
Hide comment
@hgoodman

hgoodman Feb 18, 2016

Contributor

I rolled back the index file changes and rebased the branch. Let me know if there's anything else I can do to improve index file handling. Thank you for the thorough review!

Contributor

hgoodman commented Feb 18, 2016

I rolled back the index file changes and rebased the branch. Let me know if there's anything else I can do to improve index file handling. Thank you for the thorough review!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 18, 2016

Contributor

👍

Contributor

envygeeks commented Feb 18, 2016

👍

@parkr parkr modified the milestones: 3.1.2, 3.2 Feb 18, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 18, 2016

Member

LGTM. Thanks for the PR and thanks for your thorough work! Will merge once we have 3.1.2 out.

Member

parkr commented Feb 18, 2016

LGTM. Thanks for the PR and thanks for your thorough work! Will merge once we have 3.1.2 out.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Mar 24, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 4ce50e3 into jekyll:master Mar 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Mar 24, 2016

parkr added a commit that referenced this pull request Mar 26, 2016

Merge remote-tracking branch 'origin/master' into themes
* origin/master: (65 commits)
  Update history to reflect merge of #4703 [ci skip]
  Update history to reflect merge of #4712 [ci skip]
  Highlight the test code
  Update history to reflect merge of #4640 [ci skip]
  readded "env=prod"-condition
  Update history to reflect merge of #3849 [ci skip]
  Update history to reflect merge of #4624 [ci skip]
  Update history to reflect merge of #4704 [ci skip]
  Update history to reflect merge of #4706 [ci skip]
  Checks for link file extension in tests
  Updating assets documentation
  Fix test teardown for cleaner.
  Update history to reflect merge of #4542 [ci skip]
  Add explanation of site variables in the example _config.yml
  Use double quotes in the gemfile
  Add test for creation of Gemfile by 'jekyll new'
  Add comment about github-pages
  Update history to reflect merge of #4533 [ci skip]
  Ensure Rouge closes its div/figure properly after highlighting ends.
  Add Site#config= which can be used to set the config
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment