Format markdown files and rust code blocks in markdown files #5909

ytmimi · 2023-09-12T03:15:27Z

Closes #2036

This would be a first step to resolving #5782, though this PR doesn't change anything about how rustfmt currently rewrite doc comments. Assuming this is a worthwhile change we could eventually update our doc comment rewriting code to call into these markdown formatting additions. I also want to note that these changes don't implement wrapping when wrap_comments=true is set. Given that this is more of an experimental change I didn't implement everything our current doc comment formatting handles, though these changes should handle markdown formatting more accurately since rustfmt would use pulldown_cmark to parse and format markdown documents if these changes are merged.

In the distant future I could also see this enabling rustfmt to format markdown files that are included in doc attributes, e.g #[doc = include_str!("path/to/docs.md")]

I know this looks like a massive addition, and it is, but 21k+ lines are related to JSON files and test cases that get autogenerated from those files if that makes this any more approachable 😅. @calebcartwright I know you have a preference for smaller PRs, but I figured I'd keep it all together to at least show the final vision of what I've been hacking on. I'd be happy to break this up into smaller PRs and rebase this one as those smaller PRs get merged.

ytmimi · 2023-09-12T03:16:17Z

src/formatting.rs

+        match input {
+            Input::File(ref path) if path.extension().map(|ext| ext == "md").unwrap_or(false) => {
+                let config = self.config.clone();
+                return format_markdown_code_snippets(path.clone(), &config, self);
+            }
+            _ => {}
+        }


Should this also be version gated?

The only implementaiton of the `FormatHandler` trait is the one implemented for `Session`, and in that implementation we pass the `ParseSess` to `source_file::write_file`, which also takes the `ParseSess` as an optional argument. looking at the implementation of `source_file::write_file`, the `ParseSess` is optionally used to determine the file's original line ending instead of reading the file from the file system again to do so. Given the above explanation, I don't think there's any issue changing the signature of `FormatHandler::handle_formatted_file` and the implementation of `FormatHandler for Session`.

The visibility of several private items in comments.rs were updated to `pub(crate)`. These items will help when adding support for formatting rust code blocks in markdown files.

A helper method used to determine if the code block contains rust code that should be formatted. This will help when implementing rust code block formatting in markdown files.

The `rewrite_markdown` function will take markdown as input and apply rustfmt on rust code blocks. Because of how `pulldown-cmark-to-cmark` is implemented there will be some light rewriting of the markdown file outside of code block reformatting. This mostly relates to updating newlines after markdown elements like headers and lists, and possibly updating the indentation of indented code blocks. In the future we may even consider implementing doc comment rewriting in terms of `rewrite_markdown`.

Now users can envoke rustfmt directly with a markdown file and the rust code within that file will be reformatted e.g. `rustfmt README.md`

This will make it easeir to write tests for markdown codeblock reformatting.

ytmimi · 2023-09-12T20:29:01Z

markdown/Cargo.toml

+name = "markdown"
+version = "0.1.0"
+edition = "2021"


I'm guessing I should add description, license, and repository fields. Anything else I should add?

calebcartwright · 2023-09-16T22:00:46Z

I know this looks like a massive addition, and it is, but 21k+ lines are related to JSON files and test cases that get autogenerated from those files if that makes this any more approachable 😅. @calebcartwright I know you have a preference for smaller PRs, but I figured I'd keep it all together to at least show the final vision of what I've been hacking on. I'd be happy to break this up into smaller PRs and rebase this one as those smaller PRs get merged.

Yeah that diff number made my eyes pop out like a cartoon character. At first blush it definitely feels too big to try to do in one pass, so decomposing it into the smallest atomic sets of changes will be necessary.

I do just want to re-emphasize what I said in #5782 (comment) though, namely that I don't anticipate being able to make time to dig into this, at least not until we've got things bundled up and ready to go for the 2024 style edition

ytmimi · 2023-09-19T18:23:38Z

Yeah that diff number made my eyes pop out like a cartoon character. At first blush it definitely feels too big to try to do in one pass, so decomposing it into the smallest atomic sets of changes will be necessary.

Yup, I more or less expected that

I do just want to re-emphasize what I said in #5782 (comment) though, namely that I don't anticipate being able to make time to dig into this, at least not until we've got things bundled up and ready to go for the 2024 style edition

@calebcartwright Understood. The first 3 commits are all pretty small, and lay some of the foundation for the later commits. Can we land those when broken out into their own PRs?

tgross35 · 2023-09-12T04:52:03Z

markdown/src/formatter.rs

+                    write!(self, "<!-- Dont absorb code block into list -->\n")?;
+                    write!(self, "<!-- Consider a feenced code block instead -->")?;


Suggested change

write!(self, "\n")?;

write!(self, "")?;

write!(self, "\n")?;

write!(self, "")?;

Just a small nit. To the best of my knowledge, the markdown code looks very well written.

Thanks for finding those typos. I was staring at the code for so long I never would have caught them 😅

tgross35 · 2024-04-04T06:59:42Z

@ytmimi I assume you haven't had a chance to get back to this - any way to help? I know it's not much but I could do something like create individual PRs from your smaller commits and try to chunk up the markdown crate.

Your commits really are very nicely atomic, only beef9c5 (Add markdown crate...) is big enough to possibly merit being split somehow.

ytmimi · 2024-04-04T12:46:11Z

Hey @tgross35, I haven't put more work into this PR, but I've recently been trying to expand on what I've got here in https://github.com/ytmimi/markdown-fmt. Feel free to take a look!

rustbot added the pr-not-reviewed label Sep 12, 2023

ytmimi commented Sep 12, 2023

View reviewed changes

ytmimi added 9 commits September 11, 2023 23:33

update visibility of items related to markdown code block formatting

0d4c4f6

The visibility of several private items in comments.rs were updated to `pub(crate)`. These items will help when adding support for formatting rust code blocks in markdown files.

add CodeBlockAttribute::is_formattable_rust method

1adcbf1

A helper method used to determine if the code block contains rust code that should be formatted. This will help when implementing rust code block formatting in markdown files.

Add markdown crate which hand rolls markdown formatting in rustfmt

beef9c5

Add markdown as a dependency of rustfmt

b87db9c

Allow users to format rust code in markdown code blocks

35c2f31

Now users can envoke rustfmt directly with a markdown file and the rust code within that file will be reformatted e.g. `rustfmt README.md`

Allow mardown files in system tests

079c3af

This will make it easeir to write tests for markdown codeblock reformatting.

run the markdown crate's tests during rustfmt CI

f445fee

ytmimi force-pushed the format_markdown_files_from_scratch branch from 173d9e3 to f445fee Compare September 12, 2023 03:34

ytmimi mentioned this pull request Sep 12, 2023

Using a markdown formatter for (doc) comments & wrapping #5782

Open

ytmimi commented Sep 12, 2023

View reviewed changes

tgross35 reviewed Sep 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format markdown files and rust code blocks in markdown files #5909

Format markdown files and rust code blocks in markdown files #5909

ytmimi commented Sep 12, 2023

ytmimi Sep 12, 2023

ytmimi Sep 12, 2023 •

edited

Loading

calebcartwright commented Sep 16, 2023

ytmimi commented Sep 19, 2023

tgross35 Sep 12, 2023

tgross35 Sep 19, 2023

ytmimi Sep 19, 2023 •

edited

Loading

tgross35 commented Apr 4, 2024

ytmimi commented Apr 4, 2024

		write!(self, "<!-- Dont absorb code block into list -->\n")?;
		write!(self, "<!-- Consider a feenced code block instead -->")?;

Format markdown files and rust code blocks in markdown files #5909

Are you sure you want to change the base?

Format markdown files and rust code blocks in markdown files #5909

Conversation

ytmimi commented Sep 12, 2023

ytmimi Sep 12, 2023

Choose a reason for hiding this comment

ytmimi Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

calebcartwright commented Sep 16, 2023

ytmimi commented Sep 19, 2023

tgross35 Sep 12, 2023

Choose a reason for hiding this comment

tgross35 Sep 19, 2023

Choose a reason for hiding this comment

ytmimi Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

tgross35 commented Apr 4, 2024

ytmimi commented Apr 4, 2024

ytmimi Sep 12, 2023 •

edited

Loading

ytmimi Sep 19, 2023 •

edited

Loading