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

Disabling preserved line breaks in RedCloth #284

Merged
merged 4 commits into from Jan 16, 2012

Conversation

Projects
None yet
5 participants
@laumann
Contributor

laumann commented Feb 9, 2011

I was trying out Textile and got annoyed with normal line breaks being preserved in the output, which I then found to be the standard in the Textile specification. I then found an option called fold_lines, which (by setting it to true) should do the desired thing. It turned out the real thing is called hard_breaks (which should be set to false). Anyhow I added the possibility of, in _config.yml, setting:

redcloth:
  hard_breaks: false

with the default value being true. I thought creating a redcloth grouping in _config.yml might be the smartest, because there might be more options that users would like to set in RedCloth.

PLEASE NOTE that hard_breaks is marked as deprecated (from the sources I could find), but it seems it's the only option for ignoring normal line breaks in the output.

@mojombo

This comment has been minimized.

Contributor

mojombo commented Mar 11, 2011

Could you add tests for this?

@laumann

This comment has been minimized.

Contributor

laumann commented Mar 11, 2011

I would love to, but it will take a little while, because the laptop on which I was working has been stolen (my only computer) and I'm currently travelling, but will be returning home at the end of March, then I will have to get another computer, set it up and get working. I also need to figure out how to add tests.

So, short answer, yes, but don't expect it before the middle of April. If you don't feel like waiting we can close this and I can recreate it later on - or, if you don't like it, we'll just close it :-) What say you?

@mojombo

This comment has been minimized.

Contributor

mojombo commented Mar 12, 2011

When they deprecated hard_breaks, did they introduce anything to replace it?

@laumann

This comment has been minimized.

Contributor

laumann commented Mar 12, 2011

Not as far as I could find (which is why I, in the end, went with hard_breaks), but I can look into it when I get there

@laumann

This comment has been minimized.

Contributor

laumann commented Mar 30, 2011

I added two tests in a file called test_redcloth.rb - testing hard_breaks = true/false. I hope they are "done right" so to speak - I took some inspiration from test_kramdown.rb, but I'm not sure if it's the best way to do this...

@rickardlindberg

This comment has been minimized.

rickardlindberg commented Jun 28, 2011

What is the status of this? Will it be pulled?

@laumann

This comment has been minimized.

Contributor

laumann commented Jun 29, 2011

Dunno, I just pulled mojombo/jekyll to get the latest version merged into mine, and there weren't any collisions, so I assume the reverse is possible. I can push the updated version to my fork, if that'd seal the deal (I think I might do this anyway).

On a side note, as hard_breaks is deprecated and nothing has really replaced it, one could just create their own converter plugin that utilises RedCloth, and then disable hard_breaks (although I don't think it's possible to use the .textile extension).

# if @config['redcloth']['hard_breaks'] == false
# STDERR.puts 'hards_breaks disabled'
# r.hard_breaks = false
# end

This comment has been minimized.

@mojombo

mojombo Jul 6, 2011

Contributor

Why is this code commented out? I'd prefer it be gone if it's not going to be used.

@ghost

This comment has been minimized.

ghost commented Nov 3, 2011

Is this request still active? I'd also like to have the option of removing the hard breaks in RedCloth.

As far as being deprecated, it appears to me that this commit reversed that decision despite what the comments say. I don't see an issue tracker on the RedCloth git repo or I'd ask for clarification there...

Edit: Raised the question regarding the deprecation status on the RedCloth bug tracker link

@laumann

This comment has been minimized.

Contributor

laumann commented Nov 3, 2011

That's a good question - it's technically still active, but nothing has happened in a while. I'm gonna merge the main tree into my branch to ensure there are no conflicts and push it again...

Thanks for looking into the hard_breaks deprecation question!

laumann added some commits Nov 4, 2011

Merge pull request #1 from cgroner/master
(cgroner) Missing test cases for defaults.
Have TextileConverter pass any arguments set to true in config's redc…
…loth section to RedCloth constructor as an array of symbols.

This means explicitly setting (for example):

  redcloth:
    hard_breaks: false
    lite_mode: true
    no_span_caps: true

will cause RedCloth to be invoked thusly:

  RedCloth.new(content, [:lite_mode, :no_span_caps])

(Notice that hard_breaks is ignored.) This means, however, anything set to true in the redcloth section in _config.yml _will_ be passed to RedCloth. Mayhem may ensue.
@mojombo

This comment has been minimized.

Contributor

mojombo commented Nov 27, 2011

I'm testing this now on top of the latest master and I get a test failure with Ruby 1.8.7 and RedCloth 4.2.8:

  1) Failure:
test: RedCloth with hard_breaks disabled should not generate break tags in HTML output. (TestRedCloth)
    [/Users/tom/dev/mojombo/jekyll/test/test_redcloth.rb:54:in `__bind_1322362864_863941'
     /Users/tom/Developer/.rbenv/versions/1.8.7-p352/lib/ruby/gems/1.8/gems/shoulda-2.11.3/lib/shoulda/context.rb:382:in `call'
     /Users/tom/Developer/.rbenv/versions/1.8.7-p352/lib/ruby/gems/1.8/gems/shoulda-2.11.3/lib/shoulda/context.rb:382:in `test: RedCloth with hard_breaks disabled should not generate break tags in HTML output. ']:
<"<p>line1\nline2</p>"> expected but was
<"<p>line1<br />\nline2</p>">.

Do you think you could check this out?

@laumann

This comment has been minimized.

Contributor

laumann commented Nov 28, 2011

Hi @cgroner and @mojombo,

I'm looking at it now - when I the last modified the code I wanted to incorporate all the options that one can specify for RedCloth. From http://redcloth.rubyforge.org/classes/RedCloth/TextileDoc.html there are a small number of read-writable attributes that can be specified for RedCloth. By default, all of these except hard_breaks are treated as false when not specified. It makes no sense that hard_breaks is the only attribute assumed true if omitted - rather change it to no_hard_breaks and assume it false (as the rest). Then one could write:

restrictions = Array.new
@config['redcloth'].each { |k, v| restrictions << k.to_sym if v } unless @config['redcloth'].nil?

RedCloth.new(content, restrictions).to_html 

instead of having to look for every single attribute. But I think this is a suggestion for the RedCloth maintainer...

Anyhow, I'll commit something in a bit...

@laumann

This comment has been minimized.

Contributor

laumann commented Nov 28, 2011

I'm still debating with myself about the best approach: either just stick to hard_breaks and expand as demand increases, or tackle all the possible settings now? (This raises another question: Should all these be specified in the DEFAULTS config?)

One way is to just iterate over all the elements in the config (if present), another is to retain a list of all the possible settings and ask for each of them if they have been set (the current solution). Neither seems favorable, because the first (potentially) opens a can of worms, as anything set in @config['redcloth'] section will be passed on to RedCloth. The second approach seems more secure, but takes more time (every time Jekyll is invoked it will ask about all the settings).

I also added another test case to test_redcloth.rb to ensure that the other attributes will also be set if specified - they are all treated the same, so I figured the two extra tests were enough...

Any comments on this?

@richardkmichael

This comment has been minimized.

richardkmichael commented Dec 18, 2011

Happy to see this work being done, just found myself wanting 'fold_lines' too.

I think it would be nice to config RedCloth from _config.yml, but I'd also say do the minimal work necessary to get this landed in master. Then handle the general case [if needed] later.

+1 Nice stuff!

@mojombo

This comment has been minimized.

Contributor

mojombo commented Jan 16, 2012

This looks good. I like the current approach you've taken. If we need to change it a bit later on, that shouldn't be a problem.

@mojombo mojombo merged commit d80c773 into jekyll:master Jan 16, 2012

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