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

Do not strip whitespaces and newlines when separating content from metadata #463

Merged
merged 1 commit into from Aug 16, 2014

Conversation

Projects
None yet
5 participants
@gpakosz
Copy link
Member

gpakosz commented Jul 19, 2014

When parsing items and separating content from metadata, Nanoc strips the content: https://github.com/nanoc/nanoc/blob/3.7.1/lib/nanoc/data_sources/filesystem.rb#L271

This is bogus and prevents the following Markdown from being correctly compiled:

----
title: I start with a code block
----
    this is a code block
    this is a code block

Calling #strip removes the leading spaces and breaks the code block.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2014

Coverage Status

Coverage decreased (-0.14%) when pulling b468474 on gpakosz:fix-metadata-parsing into 613f6ec on nanoc:release-3.7.x.

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Jul 19, 2014

Can you add a test for this?

@gpakosz

This comment has been minimized.

Copy link
Member Author

gpakosz commented Jul 20, 2014

So in the end I decided to only strip leading and trailing new lines. Because e.g. in Markdown, both leading spaces and trailing spaces have a meaning.

And this doesn't require to change existing tests.

@gpakosz gpakosz changed the title Do not left strip content when after having parsed metadata Strip only new-lines when separating content from metadata Jul 20, 2014

@gpakosz gpakosz changed the title Strip only new-lines when separating content from metadata Strip only new lines when separating content from metadata Jul 20, 2014

@gpakosz

This comment has been minimized.

Copy link
Member Author

gpakosz commented Jul 20, 2014

Now I'm wondering, maybe it's better to not strip anything at all, and leave everything after the metadata block intact? What do you think?

@FredyFreshFirm

This comment has been minimized.

Copy link

FredyFreshFirm commented Jul 21, 2014

Strip
20. juli 2014 14:48 skrev "Gregory Pakosz" notifications@github.com
følgende:

Now I'm wondering, maybe it's better to not strip anything at all, and
leave everything after the metadata block intact? What do you think?


Reply to this email directly or view it on GitHub
#463 (comment).

@ddfreyne ddfreyne added the bug label Aug 9, 2014

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Aug 9, 2014

I feel that not stripping anything at all is the right choice here.

@gpakosz

This comment has been minimized.

Copy link
Member Author

gpakosz commented Aug 9, 2014

So yeah I pushed a version that doesn't strip. But that breaks many tests.

So I promptly reverted that branch to the version that strips only leading and ending new lines: this is a conservative approach.

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Aug 9, 2014

I’m OK with stripping leading empty lines, but I’m not sure about trailing ones. It is common and good practice to end a file with a newline. We should be able to get away with just stripping leading empty lines, right?

@gpakosz

This comment has been minimized.

Copy link
Member Author

gpakosz commented Aug 9, 2014

Well existing tests assume trailing newlines are stripped. I'm ok with not stripping anything at all. That means changing all tests.

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Aug 9, 2014

+1 with stripping trailing newlines. If somebody complains, we can always fix it later :)

@gpakosz

This comment has been minimized.

Copy link
Member Author

gpakosz commented Aug 9, 2014

Ok by now nothing is stripped. By now content contains leading and trailing spaces / new lines.

@@ -268,7 +268,7 @@ def parse(content_filename, meta_filename, kind)
rescue Exception => e
raise "Could not parse YAML for #{content_filename}: #{e.message}"
end
content = pieces[4..-1].join.strip
content = pieces[4..-1].join

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Aug 9, 2014

Member

I like to have a blank line between the metadata section and the content, like this:

---
title: "Hello"
---

content goes here

What about calling .sub(/\A[\r\n]/, '') on the content (instead of.strip` originally)? That would allow the empty line between metadata and content and it’d still be stripped (it’d be the only thing that is stripped).

Don't strip anything when separating content from metadata
By now, content contains leading and trailing spaces / new lines
@gpakosz

This comment has been minimized.

Copy link
Member Author

gpakosz commented Aug 9, 2014

Well imho, what comes after "---" is the content. If you like to have a blank line for visual reasons, then that blank line is part of the content. Does it harm the pipeline to keep it? In the end HTML doesn't care.

So maybe it's better to consider it's part of content? Otherwise the line is blurry.

@bobthecow

This comment has been minimized.

Copy link
Member

bobthecow commented Aug 9, 2014

In YAML's opinion it's part of the content, too :)

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Aug 9, 2014

+1 for removing the blank line as well.

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Aug 15, 2014

@gpakosz Is this ready to go?

@gpakosz

This comment has been minimized.

Copy link
Member Author

gpakosz commented Aug 15, 2014

I believe it is.

Remember FilesystemUnified#create_object doesn't write a blank line before writing content anymore. Do you want the create-item to be adjusted accordingly to keep your "I like to have a blank line between the metadata section and the content" preference?

@gpakosz gpakosz added the to review label Aug 15, 2014

@ddfreyne ddfreyne removed the to review label Aug 16, 2014

ddfreyne added a commit that referenced this pull request Aug 16, 2014

Merge pull request #463 from gpakosz/fix-metadata-parsing
Strip only new lines when separating content from metadata

@ddfreyne ddfreyne merged commit a3b7f76 into nanoc:release-3.7.x Aug 16, 2014

1 check passed

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

@gpakosz gpakosz changed the title Strip only new lines when separating content from metadata Do not strip whitespaces and newlines when separating content from metadata Aug 17, 2014

ddfreyne added a commit that referenced this pull request Aug 17, 2014

@gpakosz gpakosz deleted the gpakosz:fix-metadata-parsing branch Sep 6, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.