-
Notifications
You must be signed in to change notification settings - Fork 469
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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.
Great work @zth!
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! |
Changing to draft, but please do go through it and review. |
There was a problem hiding this 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. |
There was a problem hiding this comment.
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?
Going to take the opportunity to provide a proper extraction command in tools for pulling out all code blocks, and for transforming 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). |
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
da72a1b
to
5b8ca47
Compare
@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? |
let mySet1: t<(int, int), Comparable1.identity> = %todo | ||
let mySet2: t<(int, int), Comparable2.identity> = %todo |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All assertEqual
are now ==
.
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) | ||
} | ||
``` |
There was a problem hiding this comment.
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.
There was a problem hiding this 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
…ead, since we need more info for each block than we do for formatting
7fe406b
to
f8a5a71
Compare
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:assertEqual()
to==
There are a bunch of them that cannot be formatted as of now because of:
| whatever => ...
where...
of course won't parse)|>
which now is a syntax errorWe 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?