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

skip adding binary files as posts #6344

Merged
merged 1 commit into from Sep 21, 2017

Conversation

Projects
None yet
4 participants
@Crunch09
Member

Crunch09 commented Sep 3, 2017

Instead of letting Liquid crash later in the build process IMHO we should just skip these files and inform the user.

Before:
bildschirmfoto 2017-09-03 um 14 48 26

After:
bildschirmfoto 2017-09-03 um 14 48 42

This fixes #5181

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09
Member

Crunch09 commented Sep 3, 2017

end
else
Jekyll.logger.debug "Skipping:", "#{doc.relative_path} is no valid UTF-8"

This comment has been minimized.

@Crunch09

Crunch09 Sep 3, 2017

Member

I wasn't sure if this should rather be a warning? But given that we already have an error message when this file is read i though it's good enough.

@Crunch09

Crunch09 Sep 3, 2017

Member

I wasn't sure if this should rather be a warning? But given that we already have an error message when this file is read i though it's good enough.

This comment has been minimized.

@DirtyF

DirtyF Sep 3, 2017

Member

From a user's perspective, a simple comprehensive info message would be easier to grasp:

Info:  Found a binary file in your '_posts' directory, skipping: /path/to/image.jpg
@DirtyF

DirtyF Sep 3, 2017

Member

From a user's perspective, a simple comprehensive info message would be easier to grasp:

Info:  Found a binary file in your '_posts' directory, skipping: /path/to/image.jpg

This comment has been minimized.

@parkr

parkr Sep 3, 2017

Member

Why is it a warning? Is something bad happening?

@parkr

parkr Sep 3, 2017

Member

Why is it a warning? Is something bad happening?

This comment has been minimized.

@Crunch09

Crunch09 Sep 3, 2017

Member

I guess @DirtyF changed his suggestion in the meantime to an info message? I like it, makes it easier to detect then a debug message and the existing error message does not include the information that a file has been skipped.

@Crunch09

Crunch09 Sep 3, 2017

Member

I guess @DirtyF changed his suggestion in the meantime to an info message? I like it, makes it easier to detect then a debug message and the existing error message does not include the information that a file has been skipped.

@DirtyF DirtyF added this to the v3.6.0 milestone Sep 3, 2017

@parkr parkr modified the milestones: v3.6.0, v3.7.0 Sep 21, 2017

@parkr

parkr approved these changes Sep 21, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 21, 2017

Member

Thank you!

@jekyllbot: merge +fix

Member

parkr commented Sep 21, 2017

Thank you!

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 47bcbfb into jekyll:master Sep 21, 2017

2 checks passed

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

@jekyllbot jekyllbot added bug fix labels Sep 21, 2017

parkr added a commit that referenced this pull request Sep 21, 2017

@Crunch09 Crunch09 deleted the Crunch09:issue-5181 branch Sep 22, 2017

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