Allow custom file extensions if defined in `permalink` YAML front matter #4314

Merged
merged 2 commits into from Jan 10, 2016

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Jan 5, 2016

Still a failing test. Fixes #4182 once it's passing.

What I'm trying to do is have output_ext return File.extname(doc.permalink) in the case that the doc has a permalink. This means that checking if the path ends with the output extension will always return true, thus skipping the addition.

I'm running into an issue where permalink = /slides/example-slide-7.php, but output_ext = .html, so it's being appended. If I print the document, the data hasn't been read, which is awfully strange. permalink = nil, actually, when we ask for the output_ext for the first time. Super weird.

@envygeeks, have a second to look at this? Super weird – why would @site.process in the tests not yield a fully-read document?

@parkr parkr added Bug fix labels Jan 5, 2016

@parkr parkr added this to the 3.1 milestone Jan 5, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 5, 2016

Contributor

Your problem surfaces here: https://github.com/jekyll/jekyll/blob/master/lib/jekyll/cleaner.rb#L56. Cleaner messages Document#destination before Document#read is done, resulting in Document#permalink returning nil which leads to converters.first.output_ext being messaged with document.extname instead of File.extname with document.permalink in Renderer#output_ext which leads to Document@output_ext being cached with ".html"

Contributor

envygeeks commented Jan 5, 2016

Your problem surfaces here: https://github.com/jekyll/jekyll/blob/master/lib/jekyll/cleaner.rb#L56. Cleaner messages Document#destination before Document#read is done, resulting in Document#permalink returning nil which leads to converters.first.output_ext being messaged with document.extname instead of File.extname with document.permalink in Renderer#output_ext which leads to Document@output_ext being cached with ".html"

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 5, 2016

Contributor

To add, the Document is actually being read and is fully populated. Document@output_ext just ended up being cached early leading to this order problem. Your source works, it's higher up the chain an order problem surfaces a bug.

Contributor

envygeeks commented Jan 5, 2016

To add, the Document is actually being read and is fully populated. Document@output_ext just ended up being cached early leading to this order problem. Your source works, it's higher up the chain an order problem surfaces a bug.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2016

Member

Cleaner messages Document#destination before Document#read is done, resulting in Document#permalink returning nil which leads to converters.first.output_ext being messaged with document.extname instead of File.extname with document.permalink in Renderer#output_ext which leads to Document@output_ext being cached with ".html"

I think you're right, but I'm not sure why quite yet.

Consider Site#process:

    def process
      reset
      read
      generate
      render
      cleanup
      write
      print_stats
    end

cleanup is called after read, and read has to complete before cleanup can be called. cleanup calls Cleaner#cleanup! which calls Cleaner#obsolete_files which calls Cleaner#new_dirs which calls Cleaner#new_files, which calls, for each site document, Document#destination. So I'm not really sure what's up. Especially because the failing tests use Site#process...

Member

parkr commented Jan 10, 2016

Cleaner messages Document#destination before Document#read is done, resulting in Document#permalink returning nil which leads to converters.first.output_ext being messaged with document.extname instead of File.extname with document.permalink in Renderer#output_ext which leads to Document@output_ext being cached with ".html"

I think you're right, but I'm not sure why quite yet.

Consider Site#process:

    def process
      reset
      read
      generate
      render
      cleanup
      write
      print_stats
    end

cleanup is called after read, and read has to complete before cleanup can be called. cleanup calls Cleaner#cleanup! which calls Cleaner#obsolete_files which calls Cleaner#new_dirs which calls Cleaner#new_files, which calls, for each site document, Document#destination. So I'm not really sure what's up. Especially because the failing tests use Site#process...

document: don't cache @output_ext
Fixes race issue.
Will introduce perf issues, though...

@parkr parkr changed the title from wip: allow custom extensions to Allow custom file extensions if defined in `permalink` YAML front matter. Jan 10, 2016

@parkr parkr changed the title from Allow custom file extensions if defined in `permalink` YAML front matter. to Allow custom file extensions if defined in `permalink` YAML front matter Jan 10, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2016

Member

Only test failures are the ones currently on master:

Failure:
TestKramdown#test_: kramdown when a custom highlighter is chosen should support legacy enable_coderay... for now.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:93]
Minitest::Assertion: Failed refutation, no message given
Failure:
TestKramdown#test_: kramdown when a custom highlighter is chosen should use the chosen highlighter if it's available.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:73]
Minitest::Assertion: Failed refutation, no message given
Member

parkr commented Jan 10, 2016

Only test failures are the ones currently on master:

Failure:
TestKramdown#test_: kramdown when a custom highlighter is chosen should support legacy enable_coderay... for now.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:93]
Minitest::Assertion: Failed refutation, no message given
Failure:
TestKramdown#test_: kramdown when a custom highlighter is chosen should use the chosen highlighter if it's available.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:73]
Minitest::Assertion: Failed refutation, no message given
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Jan 10, 2016

@jekyllbot: merge +bug

jekyllbot added a commit that referenced this pull request Jan 10, 2016

@jekyllbot jekyllbot merged commit 7355540 into master Jan 10, 2016

1 check failed

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

@jekyllbot jekyllbot deleted the allow-custom-php-extensions branch Jan 10, 2016

jekyllbot added a commit that referenced this pull request Jan 10, 2016

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Feb 16, 2018

Member

It looks like output_ext will be "" for HTML pages, which means an output_ext is never added?

It looks like output_ext will be "" for HTML pages, which means an output_ext is never added?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.