search

rescript-association / rescript-core

A drop-in standard library for ReScript. Intended to be familiar for JavaScript developers, easy to use, and be rich enough (without being bloated) so that you don't need to reach for anything else for typical ReScript development.
MIT License
158 stars 26 forks source link

Documentation and docstrings #25

Open zth opened 1 year ago

zth commented 1 year ago

A goal for Core is to provide a really solid developer experience. One important piece in doing that is providing good documentation, and good docstrings. For those who don't know, docstrings are the strings you prepend functions etc with to explain what they are. They show up via the editor tooling on hovering, auto completion, etc. Example from Promise.resi: https://github.com/rescript-association/rescript-core/blob/main/src/Core__Promise.resi#L11-L17

We will need help to do this. Crafting good docstrings takes time, and there's a fairly large API surface to cover.

Eventually, these docstrings will also be extracted and used for real documentation, via the doc extraction project that's in the works.

Current state

Suggested format

  1. Describe the thing in a sentence or two. Keep this short. Good to be inspired by MDN for example.
  2. Write a ReScript example in code, inside ```rescript notation. This is important and I think we should aim to have this for everything it makes sense for.

This example (and the rest in that file) is a good guide: https://github.com/rescript-association/rescript-core/blob/main/src/Core__Promise.resi#L11-L17

Workflow

I propose the following workflow:

  1. Look up a module in the project that you want to start working on.
  2. Generate a .resi file for the module, if it doesn't have one already. We aim for all modules having resi files, and to keep the documentation in there.
  3. Write in this thread that you'll start working on said module.
  4. Start working, and open a WIP PR. Commit and push often. This is so that others can pick up where you left off if needed.

I can recommend having the compiler running as you work on docstrings, and then use TempTest.res to get immediate feedback on your changes. Just use whatever you're working on and hover it in there and you'll see how the docstrings render immediately.

Editor integration

We want to make using Core an as pleasant experience as possible. Part of that is also that the editor tooling behaves the way we want it to with regards to docstrings etc. https://github.com/rescript-association/rescript-core/issues/3 this issue covers that, feel free to post things you discover in there.

Open questions

First off, please give feedback on the format, so we can ensure we cover everything we want to cover. Here are some additional open questions:

woeps commented 1 year ago

Regarding the open questions: I'd say it depends on the plans/ideas for an online api reference.

If the docstrings are the one and only truth and everything else is generates from them, I'd say yes to all of the three questions above.

If there is a plan for displaying docstrings directly on hover and offer additional resources & eplanations in some kind of "extended online api reference", my answers would change to the following:

  1. Don't suggest alternatives. Assume the dev explicitly chose the function and only show information to support the task at hand: Use the function and it's return with valid arguments. Offer some reference / link to more information.
  2. Text on hover should clearly state any side-effects: e.g. mutation - because this may be important to be remembered of, at the moment of using the function
  3. The referenced / linked content (see 1.) should support devs, when researching the right function for the usecase: Provide more elaborate explanations, links to native js docs, links to alternatives. So they are able to browse through the api, learn about differences between similar functions and choose the right function for the task.
zth commented 1 year ago

Great feedback! I agree. I think mid to long term we definitively want to provide an "extended online api reference". It's however likely that it'll be a while before that's in place, given that it takes time to build and we're a limited set of contributors with limited time at hand right now.

@woeps would you be interested in helping out shaping the initial set of docstrings? They'll set the standard going forward.

woeps commented 1 year ago

I'm definetly willing to. I just need to see how much time I'll be able to spend. 😉

I'd like to propose to separate more extensive information with a line containing only --- ("hr" element in markdown) after the most necessary information.

This would enable us to have all information at one place until more tooling / infrastructure is being worked on. This might also be simple to parse in the vs code extension and potentially truncate the extended info if necessary?

zth commented 1 year ago

Don't we all 😉

That sounds good, all for that! That'll be easy to handle and change as needed. Doc extraction tooling is coming fairly soon, so we're not ways off at least.

vasco3 commented 1 year ago

inspiration:

image
aspeddro commented 1 year ago

Should the content of the comments be indented?

https://github.com/rescript-association/rescript-core/blob/7947146895e112ad8f85427afdb7d702d04ce7ec/src/Core__Array.resi#L71-L80

Or aligned with /:

/**
`reduceReverse(xs, init, f)`

Works like `Array.reduce`; except that function `f` is applied to each item of `xs` from the last back to the first.

```rescript
Array.reduceReverse(["a", "b", "c", "d"], "", (a, b) => a ++ b) == "dcba"
```
*/
let reduceReverse: (array<'b>, 'a, ('a, 'b) => 'a) => 'a

Indenting can affect how the content is presented in the editor.

aspeddro commented 1 year ago

I'd like to propose to separate more extensive information with a line containing only --- ("hr" element in markdown) after the most necessary information.

I think we should keep it as simple as possible. These comments will be consumed by other tools that can apply this transformation.

For editor tools, comments are already rendered with good separation. In Neovim for example, it adds the --- separator before the code blocks, see https://github.com/rescript-lang/rescript-vscode/pull/619.

Maybe add a # Examples header before code block?

woeps commented 1 year ago

I think we should keep it as simple as possible. These comments will be consumed by other tools that can apply this transformation.

@aspeddro Are you saying, you're against it?

I was thinking about more extensive information, and how to seperate it. I wanted to show a real world example on the first docstring I'll work on, but for the sake of the discussion here is a fictive example:

/**
Do something very important, based on data t.

`t` is the data being operated on and `arg1` is a random string being used `arg2`-times in this operation.

**This mutates the original `t`.**

\```rescript
let x = makeT("hellp")
let result = fnName(x, ~arg1="!", ~arg2=42)
\```

---

## Explanation

This is a more thorough explanation suited for devs,
who are not familiar with this type of function.
Probably involving further meaningful examples and
step by step walkthrough if necessary. 

## References

- [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Functions) provides further usage information reagrding the bound js function
- there are some specificas to [usage in node](https://nodejs.org/api/n-api.html#napi_threadsafe_function)

## Alternatives

- `fnName` provides an immutable version of this function
- `OtherModule.otherFn` is similar, but provides more fine-grained control
- `AnotherModule.fnName` provides the same operation but on the data type `foo`

*/
external fnNameInPlace: (t, ~arg1: string, ~arg2: int) => t

Note: I needed to "escape" the example code block in the doc string to avoid weird rendering of this comment.

I was proposing, that the language server could ignore everything after (including) the line containing ---. This way we start collecting all the information now and have it available in the interface files for everyone to see. Future tooling could make use of the additional information provided. This would also mitigate your concerns regarding the rendering in some code editors. Right?

If we find consesus on the usage of this separator I'll file an issue on the language server repo.

zth commented 1 year ago

I think that's a good format @woeps . I say we go for that. Editor tooling wise this will be simple to change/fix later on, so no need to worry about that now, I'll fix that when needed. As long as there's a coherent format.

Some thoughts from me:

zth commented 1 year ago

Should the content of the comments be indented?

https://github.com/rescript-association/rescript-core/blob/7947146895e112ad8f85427afdb7d702d04ce7ec/src/Core__Array.resi#L71-L80

Or aligned with /:

/**
`reduceReverse(xs, init, f)`

Works like `Array.reduce`; except that function `f` is applied to each item of `xs` from the last back to the first.

```rescript
Array.reduceReverse(["a", "b", "c", "d"], "", (a, b) => a ++ b) == "dcba"

*/ let reduceReverse: (array<'b>, 'a, ('a, 'b) => 'a) => 'a



Indenting can affect how the content is presented in the editor.

I say we don't indent, keep it aligned to the leading / in /**.

aspeddro commented 1 year ago

I think we should keep it as simple as possible. These comments will be consumed by other tools that can apply this transformation.

@aspeddro Are you saying, you're against it?

I was thinking about more extensive information, and how to seperate it. I wanted to show a real world example on the first docstring I'll work on, but for the sake of the discussion here is a fictive example:

/**
Do something very important, based on data t.

`t` is the data being operated on and `arg1` is a random string being used `arg2`-times in this operation.

**This mutates the original `t`.**

\```rescript
let x = makeT("hellp")
let result = fnName(x, ~arg1="!", ~arg2=42)
\```

---

## Explanation

This is a more thorough explanation suited for devs,
who are not familiar with this type of function.
Probably involving further meaningful examples and
step by step walkthrough if necessary. 

## References

- [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Functions) provides further usage information reagrding the bound js function
- there are some specificas to [usage in node](https://nodejs.org/api/n-api.html#napi_threadsafe_function)

## Alternatives

- `fnName` provides an immutable version of this function
- `OtherModule.otherFn` is similar, but provides more fine-grained control
- `AnotherModule.fnName` provides the same operation but on the data type `foo`

*/
external fnNameInPlace: (t, ~arg1: string, ~arg2: int) => t

Note: I needed to "escape" the example code block in the doc string to avoid weird rendering of this comment.

I was proposing, that the language server could ignore everything after (including) the line containing ---. This way we start collecting all the information now and have it available in the interface files for everyone to see. Future tooling could make use of the additional information provided. This would also mitigate your concerns regarding the rendering in some code editors. Right?

If we find consesus on the usage of this separator I'll file an issue on the language server repo.

I had understood to add --- before the block of code (after most necessary information). It's a good format.

aspeddro commented 1 year ago

I would like to propose adding a ## Examples section after the most needed information, to improve the division.

The current state: image

zth commented 1 year ago

I think that sounds good.

aspeddro commented 1 year ago

A proposal for testing code blocks in the future.

Most docstring have checks like:

String.startsWithFrom("BuckleScript", "kle", 3) == true

However, this code is not valid. Error: Toplevel expression is expected to have unit type.

We could use assert expressions to test blocks of code.

assert(String.startsWithFrom("BuckleScript", "kle", 3))

Is it a good format or should we keep the current one?

zth commented 1 year ago

That's a good point. Ideally you should be able to run all of the examples as is, although that might not be feasible for more advanced examples that would involve a lot of boiler plate.

I'd like to avoid involving assert though. What about just wrapping it with a Console.log?

Console.log(String.startsWithFrom("BuckleScript", "kle", 3))

That would both compile, and yield some output.

aspeddro commented 1 year ago

What about just wrapping it with a Console.log?

assert throws an error, so it would be easier to debug and run tests.

although that might not be feasible for more advanced examples that would involve a lot of boiler plate.

assert can be optional. We use it for simple cases, like JS bindings. Or we can create blocks with a specific notation ```rescript ignore, like rust does not run tests in the documentation.

I'll leave it as it is. We can come back when the document extraction is more advanced.

zth commented 1 year ago

I've added docs for the Type and AsyncIterator modules:

zth commented 1 year ago

When we've covered everything in the list down to Symbol we'll make a new release and start experimenting with generating real docs (experimentation with docgen going on here: https://github.com/rescript-lang/rescript-vscode/pull/732). That means Array, Date, JSON and BigInt are left before we've covered the most important things.

Heroic effort so far all contributors! 👏

dkirchhof commented 1 year ago

I could start with doc strings for the JSON module.

zth commented 1 year ago

@dkirchhof that would be great! I'm going to start on array soon unless someone else picks it up before me.