Conversation
ppolesiuk
left a comment
ppolesiuk
left a comment
There was a problem hiding this comment.
Looks good, and I agree with proposed guidelines. I have some minor comments on what could be improved.
|
|
||
| The `@param` label is followed by a parameter name and may include an optional | ||
| description. Optional and implicit parameters must be prefixed with `~` or `?`, | ||
| just as in code. Unnamed parameters cannot be documented. |
There was a problem hiding this comment.
Does unnamed mean positional here? We code be more precise.
| just as in code. Unnamed parameters cannot be documented. | |
| just as in code. Unnamed (positional) parameters cannot be documented. |
There was a problem hiding this comment.
No. It refers to arguments in point-free style. For example function let ignore x = id cannot be fully documented, because second argument comes from identity function. I will rephrase this part a little bit
There was a problem hiding this comment.
Also wildcards may disqualify @param label. Function let ignore _ = id literally can't use @param label despite having 2 arguments
| - `@parameter Name` | ||
| - `@method Type Name` | ||
| - `@val Name` - refers to any let statement | ||
| - `@handler Name` |
There was a problem hiding this comment.
Do we need a separate @handler opening marker? As effect capabilities are first class, we can use @val.
There was a problem hiding this comment.
I don't really recall logic behind this. I think you are right, it can be deleted
| - `@data Name` - ADTs and records | ||
| - `@type Name`- Builtin types and type synonyms |
There was a problem hiding this comment.
Is there any reason to distinguish between them. The use the same namespace, so @type is sufficient.
| Supported identifiers include: | ||
| - `@data Name` - ADTs and records | ||
| - `@type Name`- Builtin types and type synonyms | ||
| - `@parameter Name` |
There was a problem hiding this comment.
Is it for parameter declarations? The documentation should be more specific.
| - `@val Name` - refers to any let statement | ||
| - `@handler Name` | ||
| - `@module Module` | ||
|
|
There was a problem hiding this comment.
It would be nice to see an example here.
| or multi-line. They always begin with a double `##` symbol and must start in | ||
| the first column of an empty line. |
There was a problem hiding this comment.
This contradicts with what is said for constructor documentation. I think, the ## should be indented to the same column as the documented entity.
Moreover, it should be clear that a docstring starts with ## or {## (from what is said, it is not clear how to start multiline docstring).
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive documentation guidelines for the standard library, establishing a "code-first" approach where documentation is embedded directly in code through docstrings and then extracted by specialized tooling.
Key Changes
- Adds detailed documentation format specifications including single-line and multi-line docstring syntax with
##markers - Defines documentation guidelines for various code elements (functions, methods, ADTs, records, types) with support for
@param,@asserts, and@returnslabels - Provides concrete examples demonstrating documentation best practices for builtin types (
Int64) and complex functions (foldRighti1)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/conv/documentation.md | New documentation guidelines file covering format specifications, scoping mechanisms, ADT/record documentation, style guidelines, and practical examples |
| src/SUMMARY.md | Adds entry for the new Code Documentation section to the table of contents |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| closing marker of a multiline comment can appear either on a separate line or | ||
| immediately after the final line of the docstring body. | ||
|
|
||
| During the process of generating doc pages, all docstring are concatenated |
There was a problem hiding this comment.
Spelling error: "docstring" should be plural "docstrings" to match the subject-verb agreement with "are".
| During the process of generating doc pages, all docstring are concatenated | |
| During the process of generating doc pages, all docstrings are concatenated |
| Specifically, don't write descriptions like "`map` maps ..." and | ||
| "`append` appends ...". | ||
| - When optional parameters are involved, describe default behavoiur | ||
| explicitely. |
There was a problem hiding this comment.
Spelling error: "explicitely" should be "explicitly".
| explicitely. | |
| explicitly. |
|
|
||
| When in doubt, feel free to lookup examples of other languages' documentation. | ||
|
|
||
| *Remeber, do not plagiariase!* |
There was a problem hiding this comment.
Spelling error: "Remeber" should be "Remember".
| *Remeber, do not plagiariase!* | |
| *Remember, do not plagiariase!* |
|
|
||
| When in doubt, feel free to lookup examples of other languages' documentation. | ||
|
|
||
| *Remeber, do not plagiariase!* |
There was a problem hiding this comment.
Spelling error: "plagiariase" should be "plagiarize" (or "plagiarise" in British English).
| *Remeber, do not plagiariase!* | |
| *Remember, do not plagiarize!* |
| be written separately and explicitly linked to the intended code element using | ||
| an identifier. | ||
|
|
||
| In such cases, docstrings are mandatorily multiline and apropriate identifiers |
There was a problem hiding this comment.
Spelling error: "apropriate" should be "appropriate".
| In such cases, docstrings are mandatorily multiline and apropriate identifiers | |
| In such cases, docstrings are mandatorily multiline and appropriate identifiers |
|
|
||
| Supported identifiers include: | ||
| - `@data Name` - ADTs and records | ||
| - `@type Name`- Builtin types and type synonyms |
There was a problem hiding this comment.
Missing space after the hyphen. Should be "- @type Name - " for consistency with other list items.
| - `@type Name`- Builtin types and type synonyms | |
| - `@type Name` - Builtin types and type synonyms |
| - Avoid using the function name as a noun and a verb in the same sentence. | ||
| Specifically, don't write descriptions like "`map` maps ..." and | ||
| "`append` appends ...". | ||
| - When optional parameters are involved, describe default behavoiur |
There was a problem hiding this comment.
Spelling error: "behavoiur" should be "behaviour" (or "behavior" in American English).
| - When optional parameters are involved, describe default behavoiur | |
| - When optional parameters are involved, describe default behaviour |
| - Document edge cases of a function. | ||
| - Do not document types of a function and arguments, unless necessary. All type | ||
| annotations will be added by a documentation tool. | ||
| - Doc lines, just as code, can not exceed width of 80 characters. |
There was a problem hiding this comment.
Spelling error: "can not" should be "cannot" (one word when expressing inability).
| - Doc lines, just as code, can not exceed width of 80 characters. | |
| - Doc lines, just as code, cannot exceed width of 80 characters. |
|
|
||
| ### Int64 | ||
|
|
||
| `Int64` is a builtin type, which means that the there is no declaration |
There was a problem hiding this comment.
Grammatical issue: remove "the" before "there" - should be "which means that there is".
| `Int64` is a builtin type, which means that the there is no declaration | |
| `Int64` is a builtin type, which means that there is no declaration |
No description provided.