Skip to content

Experimental command to format code blocks embedded in docstrings #7598

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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

zth
Copy link
Collaborator

@zth zth commented Jul 4, 2025

Adds an experimental command to tools for formatting ReScript code blocks in docstrings.

I put it under tools just because that was the easiest place to put it. Feedback gladly accepted on where to put it.
I ran formatting on Stdlib_Result.resi so you can see an example of the results easily.

@zth zth marked this pull request as ready for review July 4, 2025 12:53
@zth zth requested review from cknitt, nojaf and fhammerschmidt July 4, 2025 12:54
@zth
Copy link
Collaborator Author

zth commented Jul 4, 2025

Looking in to why CI fails.

Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Looks promising!
Will play with this once CI is green.

lineCount := currentLine + 1;
match (lines, !currentCodeBlockContents) with
| l :: rest, None ->
if String.trim l = "```rescript" then (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe check with a lowercase compare?
Allowing ReScript as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lowercase language identifier is the standard for tagging md code blocks. But we should probably add support for ```res as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels more like the thing you encountered the most in the wild rather than a hard standard.
https://github.github.com/gfm/#info-string does not mention casing.

@zth zth force-pushed the tools-format-code-blocks-in-docstrings branch from f7f994f to dc2cc93 Compare July 4, 2025 16:17
Copy link

pkg-pr-new bot commented Jul 4, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7598

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7598

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7598

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7598

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7598

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7598

commit: f4f778b

@zth
Copy link
Collaborator Author

zth commented Jul 4, 2025

@nojaf some tweaking left, but please do play around with it if you want!

@zth zth requested a review from cristianoc July 4, 2025 18:17
@nojaf
Copy link
Collaborator

nojaf commented Jul 6, 2025

Just played with this, looks good!
One thing I do wonder is if we could configure the max line length?
I found the original more readable, and a smaller max line length could maybe be beneficial.

image

Not feeling strong about it, though.

@fhammerschmidt
Copy link
Member

Just played with this, looks good!
One thing I do wonder is if we could configure the max line length?
I found the original more readable, and a smaller max line length could maybe be beneficial.

image

Not feeling strong about it, though.

That is probably a case that should be covered by smart printing - when there is a newline between any two labeled args, break.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Looks great.
Left some minor comments.

Is the idea that this might make it to default code formatting at some point?

in
let astMapped = mapper.structure mapper structure in
Ok
( Res_printer.print_implementation
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit pre-existing: there are many places in the code base where default_print_width is passed
I guess it would be cleaner for print_implementation to take an optional width argument.

end
let formatRescriptCodeBlocks content ~transformAssertEqual ~displayFilename
~addError ~markdownBlockStartLine =
let open Cmarkit in
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps it might be more readable having all uses of Cmarkit explicit

hadCodeBlocks := true;

let currentLine =
meta |> Meta.textloc |> Textloc.first_line |> fst |> Int.add 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

+ 1 less clever more explicit?

| ["-h"] | ["--help"] -> logAndExit (Ok formatDocstringsHelp)
| path :: args -> (
let isStdout = List.mem "--stdout" args in
let transformAssertEqual = List.mem "--transform-assert-equal" args in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this option is only mentioned here, and not shown in help or used in tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it was more a PoC to start out. But I think I'll add it to the help text (and tests) because it'll be useful for the stdlib stuff.

let block _m = function
| Block.Code_block (codeBlock, meta) -> (
match Block.Code_block.info_string codeBlock with
| Some ((("rescript" | "res"), _) as info_string) ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we ever have resi code blocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we could theoretically. Might be good to add support for it, I'll add it.

let formattedContents, hadCodeBlocks =
formatRescriptCodeBlocks ~transformAssertEqual ~addError
~displayFilename
~markdownBlockStartLine:pexp_loc.loc_start.pos_lnum contents
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know that a code block always takes the whole line? I guess it's a safe assumption to make, at least after normal formatting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think this should be fine. This is the line in the full source file where the markdown block starts (so the string expr itself). And even if the comment string is a single line, it'll be correct.

But, right now we only process fenced code blocks, and I believe they have to be multiline (at least one for the fenced start, and at least one for the fenced stop, + at least one for code then). So it should be fine.

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.

4 participants