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

kramdown converter: Add list_indent option #753

Closed

Conversation

cabo
Copy link
Contributor

@cabo cabo commented Apr 21, 2022

Some markdown dialects need to have nested lists indented by 4 spaces.
The kramdown converter writes the 2 spaces needed by kramdown.
The option list_indent allows setting this to 4 so that other markdown
readers can use the output.

Some markdown dialects need to have nested lists indented by 4 spaces.
The kramdown converter writes the 2 spaces needed by kramdown.
The option list_indent allows setting this to 4 so that other markdown
readers can use the output.
@gettalong
Copy link
Owner

Have you tested whether such output can successfully be parsed by kramdown again? Could you add test files?

Generally, I'm not against such an option as long as the output is something kramdown can parse again.

@cabo
Copy link
Contributor Author

cabo commented Apr 21, 2022

You already have test/testcases/block/08_list/nested.text that uses a list indent of 4, which appears to be processed fine.
If I want to expand on this; how would I feed the option list_indent: 4 into the testing mechanism and create files that are specifically checked with that option?
(Of course, the html➔kramdown➔html tests could all be run with the the option as well as without.)

@gettalong
Copy link
Owner

Yeah, that test case is working but is rather simple. When using an indentation of four, you might get wrong results, e.g.:

* list with implicit indentation of 2

        code block with 8 spaces in front

->

<ul>
  <li>
    <p>list with implicit indentation of 2</p>

    <pre><code>  code block with 8 spaces in front
</code></pre>
  </li>
</ul>

Note the two spaces at the start of the code tag. You would at least need to change the list item marker to also say that the indentation is four by using e.g. an asterisk with three spaces.

To use special options for some test cases, create an <name>.options file and specify the options in YAML format there, see test/testcases/block/03_paragraph/with_html_to_native.options.

@cabo
Copy link
Contributor Author

cabo commented Apr 21, 2022

OK, I added a {list_indent: 4} version to test_files.rb:

@@ -197,6 +197,9 @@ class TestFiles < Minitest::Test
         kdtext = Kramdown::Document.new(File.read(text_file), options).to_kramdown
         html = Kramdown::Document.new(kdtext, options).to_html
         assert_equal(tidy_output(File.read(html_file)), tidy_output(html))
+        kdtext4 = Kramdown::Document.new(File.read(text_file), options.merge({list_indent: 4})).to_kramdown
+        html = Kramdown::Document.new(kdtext4, options).to_html
+        assert_equal(tidy_output(File.read(html_file)), tidy_output(html))
       end
     end
   end

But I can't use run_tests.rb without getting the equivalent of

Testing ./testcases/block/03_paragraph/with_html_to_native.text                       /Users/cabo/.homebrew/Cellar/ruby/3.1.1/lib/ruby/3.1.0/psych/class_loader.rb:99:in `find': Tried to load unspecified class: Symbol (Psych::DisallowedClass)

(appears to trigger on .options file.)

I can do a rake but I'm not sure that triggers the test_files.rb addition.
(BTW, your dev dependencies are missing the stringex gem.)

@cabo
Copy link
Contributor Author

cabo commented Apr 21, 2022

(BTW, your dev dependencies are missing the stringex gem.)

Well, maybe not, but I still had to manually install it to get rid of 5 errors.

(I still get 19 Failures around rouge.)

@cabo
Copy link
Contributor Author

cabo commented Apr 21, 2022

Good point on the codeblock indentation, apart from that I also got an unrecognized header.
So I added the right amount spacing behind '*' (ul) and ':' (dl).

I like how running the test for list_indent=4 uncovered these additional problems, so I committed the somewhat redundant test code. I can of course refactor that, but that will lose the symmetry.

@cabo cabo force-pushed the kramdown-converter-list-indent-option branch from 28c3334 to 5051f73 Compare April 21, 2022 23:18
@gettalong
Copy link
Owner

@cabo Thanks for the changes! I have squashed your commits together and merged the result.

@gettalong gettalong closed this Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants