Skip to content

Format docstrings with the new format tools command #7623

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

Merged
merged 16 commits into from
Jul 12, 2025

Conversation

zth
Copy link
Collaborator

@zth zth commented Jul 10, 2025

This runs the new tools format-codeblocks command on the stdlib and formats all docstrings that can be formatted directly without any modification. It does the following:

  • Formats all ReScript code blocks in docstrings
  • Transforms assertEqual() to ==

There are a bunch of them that cannot be formatted as of now because of:

  1. Syntax errors (| whatever => ... where ... of course won't parse)
  2. Uses pipe last |> which now is a syntax error

We need to resolve both of these if we want to be able to check formatting in CI, and make it a part of make format (which we should).

For 1) we'll just fix them. Let's do that in a separate PR.
For 2) my personal opinion is we just remove the offending examples/docstrings if it doesn't make sense to convert them to the regular pipe. This goes for the old Js.Array etc modules, which are ancient at this point.

Thoughts?

@zth zth requested review from tsnobip, cknitt and fhammerschmidt July 10, 2025 08:18
@@ -37,7 +37,7 @@ Float constants.
module Constants: {
/**
The special value "Not a Number"
See [`NaN`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NaN) on MDN.
See [`NaN`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NaN) on MDN.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why this whitespace is removed.

Copy link
Member

Choose a reason for hiding this comment

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

that looks like a bug of the formatter when there's some indentation and the line is too long I guess?

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. This has to be Cmarkit that does that formatting. Probably something we'll need to live with, but will investigate.

@cristianoc cristianoc requested a review from Copilot July 10, 2025 08:21
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@cristianoc cristianoc requested a review from Copilot July 10, 2025 08:22
Copilot

This comment was marked as outdated.

@tsnobip
Copy link
Member

tsnobip commented Jul 10, 2025

Great work @zth!

For 2) my personal opinion is we just remove the offending examples/docstrings if it doesn't make sense to convert them to the regular pipe. This goes for the old Js.Array etc modules, which are ancient at this point.

I agree with you, these APIs use t-last (hence the use of pipe-last), I'm not sure it's worth documenting them anymore, they're very inconvenient to use now!

@zth zth marked this pull request as draft July 10, 2025 08:35
@zth
Copy link
Collaborator Author

zth commented Jul 10, 2025

Changing to draft, but please do go through it and review.

Copy link
Member

@tsnobip tsnobip left a comment

Choose a reason for hiding this comment

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

great work! That was the last remaining part to have synced formatted and tested documentation!

@@ -37,7 +37,7 @@ Float constants.
module Constants: {
/**
The special value "Not a Number"
See [`NaN`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NaN) on MDN.
See [`NaN`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NaN) on MDN.
Copy link
Member

Choose a reason for hiding this comment

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

that looks like a bug of the formatter when there's some indentation and the line is too long I guess?

@zth
Copy link
Collaborator Author

zth commented Jul 10, 2025

Going to take the opportunity to provide a proper extraction command in tools for pulling out all code blocks, and for transforming == back to assertEqual, so we can power the DocTests with that.

We're getting some syntax error from extraction here so it feels better to do that so we have proper AST driven extraction rather than the current JS based one we have now, that can be a bit fragile (as shows).

Copy link

pkg-pr-new bot commented Jul 11, 2025

Open in StackBlitz

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/win32-x64

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

commit: f8a5a71

@zth zth force-pushed the format-stdlib-docstrings branch from da72a1b to 5b8ca47 Compare July 11, 2025 13:01
@zth
Copy link
Collaborator Author

zth commented Jul 11, 2025

@cristianoc could you take another look at the changes I made to support proper code block extraction via tools?

@aspeddro could you review the changes I made to DocTests.res, to use the new code block extractor?

Comment on lines +180 to +181
let mySet1: t<(int, int), Comparable1.identity> = %todo
let mySet2: t<(int, int), Comparable2.identity> = %todo
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did this so it could be parsed/compiled.

@@ -24,7 +24,7 @@ Return the size of the array
## Examples

```rescript
assertEqual(Belt.Array.length(["test"]), 1)
Belt.Array.length(["test"]) == 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All assertEqual are now ==.

Comment on lines -29 to -53

Here is an example to find the sum of squares of all even numbers in an array.
Without pipe last, we must call the functions in reverse order:

## Examples

```rescript
let isEven = x => mod(x, 2) == 0
let square = x => x * x
let result = {
open Js.Array
reduce(\"+", 0, map(square, filter(isEven, [5, 2, 3, 4, 1])))
}
```

With pipe last, we call the functions in the “natural” order:

```rescript
let isEven = x => mod(x, 2) == 0
let square = x => x * x
let result = {
open Js.Array
[5, 2, 3, 4, 1] |> filter(isEven) |> map(square) |> reduce("+", 0)
}
```
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just went ahead and removed these since the API is ancient and |> is a parse error in v12.

@zth zth marked this pull request as ready for review July 11, 2025 13:11
@zth zth requested a review from cristianoc July 11, 2025 13:17
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Great stuff! 🎉

@@ -68,7 +68,7 @@ external isFinite: float => bool = "isFinite"
/**
Formats a `float` using exponential (scientific) notation. Return a
`string` representing the given value in exponential notation. Raise
RangeError if digits is not in the range [0, 20] (inclusive). See [`toExponential`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toExponential) on MDN.
RangeError if digits is not in the range \[0, 20\] (inclusive). See [`toExponential`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toExponential) on MDN.
Copy link
Member

Choose a reason for hiding this comment

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

Why are we getting these backslashes now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Must be something Cmarkit does. I'll have a look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So while this seems unnecessary, it's done by Cmarkit (the lib we use to render) and doesn't hurt rendering in the editor. So, I think we can leave this as is, and if Cmarkit improves (escapes less) it'll be handled automatically by a reformat.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's to make it less likely to be incorrectly interpreted as links.

@@ -164,7 +164,7 @@ external atanh: float => float = "atanh"

/**
Returns the angle (in radians) of the quotient `y /. x`. It is also the angle
between the *x*-axis and point (*x*, *y*). See
between the *x*\-axis and point (*x*, *y*). See
Copy link
Member

Choose a reason for hiding this comment

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

Same here, something is escaped too much

@zth zth force-pushed the format-stdlib-docstrings branch from 7fe406b to f8a5a71 Compare July 11, 2025 17:59
@cristianoc cristianoc requested a review from Copilot July 12, 2025 20:03
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.

Go for it!

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR integrates the new format-codeblocks command into the tooling and applies it across docstrings in the stdlib. It:

  • Adds a readFile helper and consolidates file-reading logic in tools.ml.
  • Enhances the FormatCodeblocks module to format ReScript code blocks and convert assertEqual(a, b) to a == b.
  • Updates test scripts and fixtures to use the format-codeblocks and extract-codeblocks commands, and revises expected outputs accordingly.

Reviewed Changes

Copilot reviewed 94 out of 98 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tools/src/tools.ml Added readFile and unified file loading; enhanced code-block formatting logic.
tests/tools_tests/test.sh Updated test script to run format-codeblocks and extract-codeblocks.
tests/tools_tests/src/expected/**/* Revised expected fixtures to reflect new formatting (use ==, JSON changes).
tests/tools_tests/src/docstrings-format/* Added sample files for format-codeblocks and extract-codeblocks testing.
runtime/**/*.resi Reformatted inline examples in doc comments to use == and updated regex syntax.
Comments suppressed due to low confidence (1)

tests/tools_tests/test.sh:28

  • The for loop starting here is never closed with a matching done. Add a done after the fi on line 33 to properly terminate the loop.
for file in src/docstrings-format/*.{res,resi,md}; do

@@ -677,6 +677,23 @@ let extractEmbedded ~extensionPoints ~filename =
])
|> List.rev |> array

let readFile path =
let ic = open_in path in
Copy link
Preview

Copilot AI Jul 12, 2025

Choose a reason for hiding this comment

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

Use open_in_bin instead of open_in to read files in binary mode and avoid automatic newline conversions on Windows.

Suggested change
let ic = open_in path in
let ic = open_in_bin path in

Copilot uses AI. Check for mistakes.

@zth zth merged commit b9bd407 into master Jul 12, 2025
27 checks passed
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.

5 participants