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

Fix line wrapping within inline code #130

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chriskrycho
Copy link

@chriskrycho chriskrycho commented Dec 3, 2024

  • Adds a failing test.
  • Fixes the issue by having gen_code call gen_str with the string it builds, before returning it.

Fixes #129.

Note that the failing test results in a timeout after the panic, rather than failing the test suite:

Test output
$ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.03s
     Running unittests src/lib.rs (target/debug/deps/dprint_plugin_markdown-bb4603beba64faa3)

running 13 tests
test generation::cmark::parsing::parse_link_reference_definitions::tests::it_finds_link_reference_with_new_line_after ... ok
test generation::cmark::parsing::parse_link_reference_definitions::tests::it_finds_multiple_link_references ... ok
test generation::cmark::parsing::parse_link_reference_definitions::tests::it_finds_link_reference ... ok
test generation::cmark::parsing::parse_link_reference_definitions::tests::it_parses_empty_string ... ok
test configuration::builder::tests::use_markdown_defaults_when_global_not_set ... ok
test configuration::builder::tests::handle_global_config ... ok
test generation::cmark::parsing::parse_image::test::it_should_parse_image ... ok
test generation::metadata::test::it_should_strip_plus_plus_plus_header ... ok
test generation::metadata::test::it_should_strip_yaml_header ... ok
test configuration::builder::tests::check_all_values_set ... ok
test generation::utils::test::it_should_find_list_words ... ok
test generation::utils::test::should_unindent ... ok
test format_text::test::strips_bom ... ok

test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/newline_test.rs (target/debug/deps/newline_test-2293c22b8eb4ea3a)

running 4 tests
test test_issue35_convert_two_spaces_end_of_line_to_hard_break ... ok
test test_issue35_ignore_two_spaces_before_hard_break ... ok
test test_issue22_with_carriage_return_line_feeds ... ok
test test_issue26_with_carriage_return_line_feeds ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/spec_test.rs (target/debug/deps/specs-6c950b6b2c6409a0)

     Running specs::BlockQuotes

test specs::BlockQuotes::BlockQuotes_TextWrap_Always ... (6ms)
  should format ok
  should wrap a line when it exceeds the line width ok
test specs::BlockQuotes::Callouts ... (8ms)
  should format ok
  should format when just a callout ok
  should format when has blank line ok
test specs::BlockQuotes::BlockQuotes_All ... (9ms)
  should format ok
  should not wrap a line when it exceeds the line width and text wrapping is to maintain ok
  should keep blank lines in the middle ok

     Running specs::Code

thread '<unnamed>' panicked at /Users/chris/.cargo/registry/src/index.crates.io-6f17d22bba15001f/dprint-core-0.66.2/src/formatting/printer.rs:623:7:
Debug panic! Found a newline in the string. Before sending the string to the printer it needs to be broken up and the newline sent as a PrintItem::NewLine. `this
out`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Panic in spec 'should wrap inline code across lines' in ./tests/specs/Code/Code_Inline_TextWrap_Always.txt

thread '<unnamed>' panicked at /Users/chris/.cargo/registry/src/index.crates.io-6f17d22bba15001f/dprint-development-0.10.1/src/spec_helpers.rs:152:27:
called `Result::unwrap()` on an `Err` value: Any { .. }
test specs::Code::Code_Inline ... (14ms)
  should format inline blocks ok
  should properly space when last on line with soft break ok
  should properly space when first line after soft break ok
  should format code inside text parens ok
  should format double backticks as-is when it includes backticks ok
  should leave spaces around backticks inside double backticks ok
test specs::Code::Code_Inline_TextWrap_Always has been running for more than 60 seconds

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2024

CLA assistant check
All committers have signed the CLA.

chriskrycho added a commit to rust-lang/book that referenced this pull request Dec 5, 2024
Correctly applies line wrapping to inline code blocks. See [the
issue][129] and [fix PR][fix] for more details.

[issue]: dprint/dprint-plugin-markdown#129
[fix]: dprint/dprint-plugin-markdown#130
@chriskrycho
Copy link
Author

Friendly ping/nudge here @dsherret! I’m sure you’re busy, but would love to see this upstream.

The behavior at the time of introducing this test is:

- An initial line wrap for a too-long line is correct.
- When the plugin finds inline code wrapping across a line and decides
  to rewrap it, it inserts an additional `>` at the end of the leading
  line.

This is a *new* failure mode introduced by the previous change, which
added the first support for handling wrapping inline code across lines
at all.
Comment on lines +29 to +41
!! should rewrap inline code across lines in blockquote !!
> Testing `this
> out`.

[expect]
> Testing `this out`.

!! should rewrap inline code across lines which are too long in blockquote !!
> This is some long text, which will have a bit of inline code across the `80 character limit` and then more after that.

[expect]
> This is some long text, which will have a bit of inline code across the `80
> character limit` and then more after that.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discovered this failure mode while testing this PR against the Rust book, where there are a few of these. Should hopefully have a fix in for this tomorrow morning!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some breadcrumbs here for myself, given I do not expect to get back to this till January—I spent some time chasing this (after beating my head against the wall that is getting useful debugging set up for Rust 😑) and concluded so far that it is down to something amiss in how dprint-plugin-markdown is using the result of parsing from pulldown-cmark, but haven’t gotten further than that.

Specifically, it ends up seeing an inline code snippet that wraps across a line in a blockquote as including the > character. It will correctly introduce the wrapping for a too-long line, but is not stable because once it has done so, on the next formatting pass it will then interpret it as part of the inline code instead of as a blockquote marker.

A side observation: this is just a challenging issue. Here is a simple sample that I used for testing pulldown-cmark itself locally:

> This is some `inline
> code`. And it gets
> *rendered* just fine.
> The problem is thus
> not `pulldown-cmark`
> but something in how
> `dprint` uses it.

Notice how GitHub’s (line-wrapping-aware) renderer trips in a different way over the exact same thing, not wrapping when it should:

This is some inline code. And it gets
rendered just fine.
The problem is thus
not pulldown-cmark
but something in how
dprint uses it.

@chriskrycho chriskrycho marked this pull request as draft December 20, 2024 00:15
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.

textWrap: "always" does not rewrap contents of inline code
2 participants