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

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

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
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.

@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
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