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

Allow static files in Collections #2615

Merged
merged 4 commits into from Aug 2, 2014

Conversation

Projects
None yet
3 participants
@alfredxing
Member

alfredxing commented Jul 17, 2014

Allow StaticFiles in Collections alongside Documents. Attempts to fix #2555, #2598. Can also be modified a bit to accommodate #2592. Builds on #2603.

Please test!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 17, 2014

Member

This raises an interesting question: what is a Document? Does a document have to be text? Can we also mold the Document idea to an image file? How does iterating over a collection's documents have to change if we're only concerned about text files but have image documents?

Member

parkr commented Jul 17, 2014

This raises an interesting question: what is a Document? Does a document have to be text? Can we also mold the Document idea to an image file? How does iterating over a collection's documents have to change if we're only concerned about text files but have image documents?

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jul 17, 2014

Member

From what I know, a Document needs to have content. Will that work properly for binary files?

BTW, the build failed because of this test, as in my implementation, any file without a YAML header is considered static. Is that correct?

Member

alfredxing commented Jul 17, 2014

From what I know, a Document needs to have content. Will that work properly for binary files?

BTW, the build failed because of this test, as in my implementation, any file without a YAML header is considered static. Is that correct?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 17, 2014

Member

We'd rather see the YAML front-matter optional if at all possible.

Member

parkr commented Jul 17, 2014

We'd rather see the YAML front-matter optional if at all possible.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jul 17, 2014

Member

I chose the YAML front-matter 'threshold' since actually testing if the file is binary seems to be pretty hard.

I'll see what I can do with your suggestion about Documents though. The only downside I can see with that is we'll have to potentially do the same thing with Posts, and that will mean these two classes might be representing a bit too much.

Member

alfredxing commented Jul 17, 2014

I chose the YAML front-matter 'threshold' since actually testing if the file is binary seems to be pretty hard.

I'll see what I can do with your suggestion about Documents though. The only downside I can see with that is we'll have to potentially do the same thing with Posts, and that will mean these two classes might be representing a bit too much.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 17, 2014

Member

I guess they can use a data collection for non-yaml stuff (e.g. JSON), but it sort of defeats the purpose of having an all-encasing collection. Ideally we just have the renderer as the document "hey, can you be converted/rendered with liquid?" And proceed as expected from there on.

Member

parkr commented Jul 17, 2014

I guess they can use a data collection for non-yaml stuff (e.g. JSON), but it sort of defeats the purpose of having an all-encasing collection. Ideally we just have the renderer as the document "hey, can you be converted/rendered with liquid?" And proceed as expected from there on.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jul 17, 2014

Member

I seemed to have figured most of it out (it's quite simple actually), but there's this one error coming from this line: Error reading file /Users/alfred/Desktop/test/_test/test.jpg: invalid byte sequence in UTF-8. It's because Jekyll's defaults read files as UTF-8, and we're explicitly catching the error.

This works though: @content = File.open(path, "rb") { |f| f.read } but it completely disregards the options passed in. Any ideas?

Member

alfredxing commented Jul 17, 2014

I seemed to have figured most of it out (it's quite simple actually), but there's this one error coming from this line: Error reading file /Users/alfred/Desktop/test/_test/test.jpg: invalid byte sequence in UTF-8. It's because Jekyll's defaults read files as UTF-8, and we're explicitly catching the error.

This works though: @content = File.open(path, "rb") { |f| f.read } but it completely disregards the options passed in. Any ideas?

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jul 17, 2014

Member

OK, so there's one test that failed (it's a Liquid document without a YAML header).

I don't think only processing files with a YAML header is too much of a problem, since pages exhibit the same behaviour (pages without a header aren't processed by Liquid).

Member

alfredxing commented Jul 17, 2014

OK, so there's one test that failed (it's a Liquid document without a YAML header).

I don't think only processing files with a YAML header is too much of a problem, since pages exhibit the same behaviour (pages without a header aren't processed by Liquid).

Show outdated Hide outdated lib/jekyll/document.rb
@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jul 28, 2014

Member

The proofer seems to have stalled in the second build, but the other two passed, so the PR should be good to merge (if you want to re-run the CI build that's fine too!)

Member

alfredxing commented Jul 28, 2014

The proofer seems to have stalled in the second build, but the other two passed, so the PR should be good to merge (if you want to re-run the CI build that's fine too!)

Show outdated Hide outdated lib/jekyll/document.rb
@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 1, 2014

Member

Build failed because the git diff command executed somehow contained site files (we should look into this as well). I ran bundle exec rake locally and it does pass all of the tests.

Member

alfredxing commented Aug 1, 2014

Build failed because the git diff command executed somehow contained site files (we should look into this as well). I ran bundle exec rake locally and it does pass all of the tests.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 1, 2014

Member

git diff command executed somehow contained site files

It's either way – so if we merge a PR or push a commit containing changes to one of the site files, we will see this diff succeed and it'll proof whether we want it to or not. We should probably turn this off until we can find the right solution – I've been fighting with it for a while.

Member

parkr commented Aug 1, 2014

git diff command executed somehow contained site files

It's either way – so if we merge a PR or push a commit containing changes to one of the site files, we will see this diff succeed and it'll proof whether we want it to or not. We should probably turn this off until we can find the right solution – I've been fighting with it for a while.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 1, 2014

Member

@parkr Think I found a solution to the diff problem: #2672

Member

alfredxing commented Aug 1, 2014

@parkr Think I found a solution to the diff problem: #2672

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 1, 2014

Member

@parkr Do you want to re-run the CI build? It should pass now due to the merge of #2672.

Member

alfredxing commented Aug 1, 2014

@parkr Do you want to re-run the CI build? It should pass now due to the merge of #2672.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 1, 2014

Member

Did you rebase?

Member

parkr commented Aug 1, 2014

Did you rebase?

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 1, 2014

Member

Gosh, I need a lecture on Git. Was that the right rebase?

Edit: Oh, nevermind, I get what you mean now:

If necessary, rebase your commits into logical chunks, without errors.

Member

alfredxing commented Aug 1, 2014

Gosh, I need a lecture on Git. Was that the right rebase?

Edit: Oh, nevermind, I get what you mean now:

If necessary, rebase your commits into logical chunks, without errors.

alfredxing added some commits Jun 29, 2014

Allow static files in collections
Allow Documents to be static files so static files can exist in collections
@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 1, 2014

Member

Done rebasing!

Member

alfredxing commented Aug 1, 2014

Done rebasing!

Show outdated Hide outdated lib/jekyll/document.rb
@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 2, 2014

Member

Done! Do you want me to rebase again?

Member

alfredxing commented Aug 2, 2014

Done! Do you want me to rebase again?

Show outdated Hide outdated lib/jekyll/document.rb
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 2, 2014

Member

Awesome. The solution is looking solid. Just needs a quick test for an image file and we should be good to merge.

Member

parkr commented Aug 2, 2014

Awesome. The solution is looking solid. Just needs a quick test for an image file and we should be good to merge.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 2, 2014

Member

Tests added.

Member

alfredxing commented Aug 2, 2014

Tests added.

@parkr parkr merged commit 81807cb into jekyll:master Aug 2, 2014

1 check passed

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

parkr added a commit that referenced this pull request Aug 2, 2014

@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.